On Tue, Jul 16, 2013 at 10:22:33PM +0200, George Bosilca wrote: > Nathan, > > I read your code and it's definitively looking good. I have however few minor > issues with your patch. > > 1. MPI_Aint is unsigned as it must represent the difference between two > memory arbitrary locations. In your MPI_Type_get_[true_]extent_x you go > through size_t possibly reducing it's extent. I would suggest you used > ssize_t instead.
Ah, yes. That is correct will fix that and update my repository now. > 2. In several other locations size_t is used as a conversion base. In some of > these location there is even a comment talking about ssize_t ? Will fix this as well. > 3. We had a policy that we only export one single MPI level function per file > in the mpi directory. You changed this as some of the files exports now two > function (the original function together with the _x version). I was trying to avoid having too much duplicate code. If including both functions in the same file is not ok I will move the _x functions to their own .c files. > 4. In the OPAL datatype stuff sometimes you use size_t and sometimes ssize_t > for the same type of logic (set and get count as an example). Why? I replaced uint32_t with size_t and int32_t with ssize_t to be consistent with the original code. > 5. You change the comments in the opal_datatype.h with "question marks"? the > cache boundary must be known, it can't be somewhere between x-y bytes ago ? The problem is size_t can be either 4 or 8 bytes so there are two possible places for the cache boundary. If you prefer I can change those to use int64_t and uint64_t instead so we will know where the cache boundaries are. (or leave them as 32-bit if that is the correct answer). > 6. I'm not sure the change of nbElems from uint32_t to size_t (in > opal/datatype/opal_datatype.h) is doing what you expect? Admittedly, I changed the size of nbElems early on. I left it as 64-bit (32-bit on 32-bit platforms) to allow the creation of datatypes with more than 2^32 elements. Not sure this senario will ever occur though. > > > Btw, I have a question to you fellow MPI Forum attendees. I just can't > remember why the MPI forum felt there was a need for the > MPI_Type_get[_true]_extent_x? MPI_Count can't be bigger than MPI_Aint, so I > don't see what is the benefit of extending the > MPI_Type_get_true_extent(MPI_Datatype, MPI_Aint*, MPI_Aint*) and > MPI_Type_get_extent(MPI_Datatype, MPI_Aint*, MPI_Aint*) with the > corresponding _X versions? It was before my involement with the forum. Jeff knows better why this was done. Thanks for taking a look. I will let you know when I have fixed the ssize_t/size_t issue. -Nathan
