Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MPI C++ Datatypes: don't declare/define if no C++ support #1389

Closed

Conversation

jsquyres
Copy link
Member

Per MPI-3.1 A.1.1 p674, only define the C++ datatypes if we're actually building C++ support.

Signed-off-by: Jeff Squyres jsquyres@cisco.com

This consequence came out of the "should we define Fortran MPI datatypes in mpi.h if there's no Fortran support?" discussion. In the MPI Fortran WG discussion, it turns out that MPI-3.1 does say that the C++ datatypes should not be available in mpi.h if there's no C++ bindings. The spec unfortunately don't say anything about the Fortran datatypes -- so it'll be brought up in the March 2016 MPI Forum meeting.

This commit removes the C++ MPI Datatypes from mpi.h when there's no C++ support.

@bosilca Please review.

@jsquyres jsquyres added the bug label Feb 20, 2016
@jsquyres jsquyres added this to the v2.0.1 milestone Feb 20, 2016
@lanl-ompi
Copy link
Contributor

Test FAILed.

@jsquyres
Copy link
Member Author

Hey @bosilca, the jenkins tests failed -- and with good reason.

I #if'ed out some datatypes in datatype_module.c, which therefore throws off the ordering of the indexing in the datatype array. What's the Right way to do this -- still define the ompi_cxx_ types, but just remove the MPI_CXX_ #define's from mpi.h.in? Typing that out makes it sound like the Right thing to do -- I'll do that. Let me know what you think.

Per MPI-3.1 A.1.1 p674, only define the C++ datatypes if we're
actually building C++ support.

Signed-off-by: Jeff Squyres <jsquyres@cisco.com>
@jsquyres jsquyres force-pushed the pr/dont-need-no-stinkin-cxx-types branch from 90f2d78 to 05c1108 Compare February 20, 2016 16:14
@bosilca
Copy link
Member

bosilca commented Feb 20, 2016

Removing the defines from the mpi.h is the correct thing to do from the user perspective. However, if we want the entire system to work correctly we should also make sure that the undefined datatypes points to UNAVAILABLE. That being said, this has many implication that have not been foreseen.

  1. extern32: table 13.2 provide no support to make some type missing from the standard
  2. connecting dynamic processes will not work the way it is defined today, the would be a need for a handshake to see what is supported by the different implementations involved.
  3. Manipulating files might become a nightmare if some types are missing. It might even break their compatibility, or force users to always fallback to predefined C types, the only datatypes that are (based on you definition) always available and therefore portable.

@jsquyres
Copy link
Member Author

@bosilca I can mention these concerns in the presentation I'm making at the Forum next week about the corresponding Fortran datatypes issue.

@hjelmn
Copy link
Member

hjelmn commented Feb 23, 2016

:bot:retest:

@gpaulsen
Copy link
Member

gpaulsen commented Mar 3, 2016

I like Jeff's suggestion of keeping it it in the datatype array to maintain the indexes, but NOT define them in mpi.h.
I'm okay with all of the "bugs" that George outlined. :) Just get them out of mpi.h!

@jsquyres
Copy link
Member Author

jsquyres commented Mar 3, 2016

We present this idea to the Forum tomorrow in plenary sessions (i.e., if Fortran datatypes need to exist in mpi.h if there is no Fortran support); we'll see what they say.

@jsquyres
Copy link
Member Author

@gpaulsen
Copy link
Member

bump. Looks like not alot of concensus around MPI_*_(C2F|F2C)?

@jsquyres jsquyres modified the milestones: v2.0.1, v2.0.2 Sep 2, 2016
@jsquyres jsquyres modified the milestones: v2.x, v2.0.2 Sep 26, 2016
@jsquyres
Copy link
Member Author

Please add a Signed-off-by line to this PR's commit.

@jsquyres jsquyres modified the milestones: v2.x, v2.1.0, v3.0.0 Dec 5, 2016
@jsquyres
Copy link
Member Author

jsquyres commented Dec 5, 2016

Looks like this is not yet resolved in the MPI Forum. Bumped to v3.0.0.

@bwbarrett
Copy link
Member

According to Jeff, the forum still hasn't resolved this issue. Next meeting is Jun 14, which is way too late to hit 3.0.0. I'm going to push this to 3.1.x milestone. Someone should bring it back to v3.0.x if they disagree.

@bwbarrett bwbarrett modified the milestones: v3.1.0, v3.0.0 Mar 28, 2017
@bwbarrett bwbarrett modified the milestones: Future, v3.1.0 Sep 12, 2017
@jsquyres
Copy link
Member Author

Looks like there's still a bunch of unresolved questions on this. It doesn't seem that important, and is therefore low priority (as evidenced by this PR being open for over a year with no action). So I'm just going to close this PR without merging.

@jsquyres jsquyres closed this Sep 19, 2017
@jsquyres jsquyres deleted the pr/dont-need-no-stinkin-cxx-types branch December 18, 2020 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants