[MOAB-dev] [Itaps-users] [PATCH 1/2] iMesh_MOAB: do not allocate new memory for count=0 items

[James Porter]

On Fri, 2011-06-10 at 23:00 +0200, Jed Brown wrote:
> On Fri, Jun 10, 2011 at 22:03, James Porter <jvporter at wisc.edu> wrote:
>         It's close, but not quite equivalent. What I'm suggesting is
>         that we
>         also remove the "alloc=0" means "allocate for me" behavior
>         entirely. The
>         current patch will allocate a new array when alloc=0,
>         array=non-NULL,
>         and count>0, but I think it should error out, since:
>         
>         1) it simplifies the semantics of the arguments (alloc no
>         longer has a
>           special value to invoke "allocate for me" behavior)
>         2) it guarantees that no matter how you specify "allocate for
>         me", that
>           you have a usable array on return (i.e. you can loop over it
>         and
>           free() it) because there's only one way to specify that.
>         
>         Essentially, if array=NULL, iMesh should be able to totally
>         ignore the
>         alloc parameter. Granted, like your patch, this requires
>         callers to
>         change how they initialize their arrays, but I don't think
>         there's a way
>         around that.
> 
> Okay, I also prefer these stricter semantics, but it's a much bigger
> API change so it can't be done as casually.

As you mentioned earlier, your proposal will require people to
initialize the array to NULL in cases where the result might be an empty
array. It will still work for non-empty arrays to set alloc=0 and
nothing else, though.

In my version, this falls under case F below, and would return an error,
essentially forcing people to switch to the new "best practice" for
array initialization. I think that's a good thing, since I'm a little
worried that people won't pay attention to this change because it will
usually work and then months/years later, they'll hit the case where the
result array is empty and then things will explode. We might as well
force people to change now while it's fresh in everyone's mind, so that
it doesn't bite us later.

> Here is the proposed logic table:
> 
> 
> case   array     alloc     actual     Allocation   Result
> --------------------------------------------------------------
> A      NULL      0         0          no           success
> B      NULL      0         1+         yes          success
> C      NULL      1+        0          no           success
> D      NULL      1+        1+         no           ? [1]
> E      valid     0         0          no [2]       success
> F      valid     0         1+         no           failure [3]
> G      valid     1+        0          no           success
> H      valid     1+        1+         no           success 
> 
> 
> [1] If you don't allocate, then either SEGV or iBase_BAD_ARRAY_SIZE.
> Note that you can't actually distinguish a NULL pointer from an
> invalid pointer (via malloc(0) or otherwise). An error here is just
> being polite, but the check would be optional for a conforming
> implementation. Are you suggesting to allocate and succeeed here? That
> would be more lenient. I don't have a strong rationale either way.

I lean (not strongly) towards accepting that and allocating an array.
Even though that set of parameters isn't self-consistent, I don't think
that accepting that case hurts anything else. As long as it doesn't
cause any problems, I think we should try to be as lenient as possible
in handling arguments from the user.

This is still dependent on whether or not Fortran can set the array
pointer to NULL. I assume it can, but this wouldn't be the first time
that Fortran support prevented us from doing something nice.

- Jim