itaps-parallel technical issues in iMeshP.h

[Jason Kraftcheck]

Devine, Karen D wrote:
> Jason:  Thanks for the good comments; please see my replies below.
> All:  I just svn committed to the RPI repository updates based on Ting's and
> Jason's recent emails.
> 
> Karen
> 
> 
> On 10/2/08 3:46 PM, "Jason Kraftcheck" <kraftche at cae.wisc.edu> wrote:
> 
>> iMeshP_createPartitionAll
>>   - Why MPI_Comm*?  It is an "in" argument.  Pass by value would
>>     be better, but if passed by pointer is desired for some reason
>>     it should be const.
> 
> I'm just used to passing by pointer things that may be structures such as
> MPI_Comm.  To be more consistent with MPI, we should pass it by value.  I'll
> make the change.
> 
>> iMeshP_destroyPartitionAll
>>   - Why is 'partition_handle' inout?  What is this function returning
>>     a handle to?  Some other partition?
> 
> The partition handle should be set to an invalid value when we destroy the
> partition.
> 

Why do we do this for partition handles, and not for any other type of
handle?  E.g.:
  iMesh_deleteEnt
  iMesh_deleteEntArr
  iMesh_destroyTag
  iMesh_destroyEntSet
  iMesh_endEntIter
  iMeshP_destroyPart
and for that matter:
  free
  delete
  fclose
  etc.

I think it is better not to do things that aren't necessary for the desired
operation.  Clearing the stale handle value for the caller unnecessarily
restricts the caller.  For example, assume I want to implement a C++ class
in my application for which each instance manages a partition for me, taking
care of releasing them, etc.  Something like:
  class Partition {
    public:
    Partition( iMeshInstance instance, iMeshP_PartitionHandle handle )
      : myHandle(handle), myiMesh(instance) {}

    ~Partition() {
	int err;
        iMeshP_destroyPartitionAll(myiMesh, &myHandle, &err );
    }

    ...

    private:
       const iMeshP_PartitionHandle myHandle;
       const iMeshInstance myiMesh;
  };

The above code will not compile as is.  I want to declare the
'iMeshP_PartitionHandle myHandle' as const because I have no intention of
changing it during the life of the object, but I cannot do so because
iMeshP_destroyPartitionAll wants to clear the value when the object is
destroyed.  I could, of course, work around this by making a copy of
myHandle to pass into iMeshP_destroyPartitionAll, but why force people to
work around the API.  Let the caller worry about clearing stale handle
values if it wants to.


>>   - Why the "All" in the name?  It isn't destroying all partitions,
>>     just the passed one.  Presumably in the previous function 'All'
>>     means the 'Mesh', but unless where destroying the Mesh in this function
>>     it doesn't make sense from a consistency POV either.
> 
> We used the convention "All" to indicate that all processes must call the
> function synchronously.  In this case, all processes in a partition must
> agree to destroy the partition at the same time.  It would be bad for some
> processes to think a partition exists while other processes have destroyed
> it.
> 

I think that "allDestroyPartition" (subject before verb) would be less
confusing.  But maybe that's just me.

>> iMeshP_getPartFromPartHandle
>> iMeshP_getPartFromPartHandlesArr
>> iMeshP_getPartHandleFromPart
>> iMeshP_getPartHandlesFromPartsArr
>>   - Should these be iMeshP_getPartIdFromPartHandle(sArr)
>>                                   ^^
>>        and iMeshP_getPartHandleFromPartId(sArr) ?
>>                                        ^^
> 
> Fine with me; they are probably clearer that way.
> 
> 
>> void iMeshP_iPushTagsEnt
>>   - If 'entities' is an input, it should be const.
> 
> Shall do.  Are all input values supposed to be const?  Or just pointers?

The pointers should be.  It doesn't really matter for other inputs.  For a
pointer, specifying it const means that the calling function cannot change
the pointed-to data.  For non-pointer (and non-reference args in C++)
declaring the arg as const doesn't provide any meaningful guarantees to the
caller.  The args are passed by copy.  They cannot be anything but "in",
regardless of a 'const' designator.  If the argument is "in" then the caller
shouldn't change it.  The C/C++ convention is to use const as necessary to
force the "callee can't change it" part, implying the "in" property.

- jason