Specification #188

iMesh: consider void* instead of void** for generic tag data routiines

Added by Mark Miller over 6 years ago. Updated over 6 years ago.

Status:ResolvedStart date:03/21/2011
Priority:NormalDue date:
Assignee:Mark Miller% Done:

0%

Category:-Estimated time:0.10 hour
Target version:1.2Spent time:0.10 hour

Description

In changing get/set generic data data routines from char** to void**, we introduced a backward incompatibility that we didn't have to introduce; every caller now has to explicitly cast their args to (void**) to get a successful compilation.

We didn't have to do it this way. We could have used void* instead of void** in the function sigantures in the API specification. I guess this is because there is no reason to distinguish a void pointer and a pointer to a void pointer.

Jed Brown suggeste/requested this change. However, in some sense the cat is out of the bag now. Anyone compiling against 1.2 headers is going to have to have fixed this. So, I am not sure I see much point in now going reversing that. On the other hand, if we do revert to void* anyone who hasn't already moved to 1.2 won't be hit by it in moving to 1.3 or later.

History

#1 Updated by Tim Tautges over 6 years ago

IMO, we should change this now, since it mostly affects us while changing it later will affect all users.

#2 Updated by Mark Miller over 6 years ago

We can change now on trunk. We might be able to get away changing ITAPS header file on RC too. However, I don't think we can expect impls to respond prior to 1.2 release. And, what happens to any code that is already explicitly casting (void**)? Will compiler be ok with passing that to a void*?

I am also a little concerned if when user's see void* as formal argument type, they'll really understand we want a pointer to a pointer.

    /**\brief  Get the value of a tag of arbitrary type on an entity set
     *
     * ....
     * \param *tag_value Pointer to tag data array being queried
     * ....
     */
  void iMesh_getEntSetData(...
                           /*inout*/ void* tag_value,
                           ...

#3 Updated by E. Seegyoung Seol over 6 years ago

Mark Miller wrote:

We can change now on trunk. We might be able to get away changing ITAPS header file on RC too. However, I don't think we can expect impls to respond prior to 1.2 release. And, what happens to any code that is already explicitly casting (void**)? Will compiler be ok with passing that to a void*?

I am also a little concerned if when user's see void* as formal argument type, they'll really understand we want a pointer to a pointer.

Why don't we go ahead with "void*"? I don't think fifth graders will ever use the itaps interface.

#4 Updated by James Porter over 6 years ago

Mark Miller wrote:

However, I don't think we can expect impls to respond prior to 1.2 release.

For MOAB, it'll be no problem, and we should be able to fix this before 1.2.

And, what happens to any code that is already explicitly casting (void**)? Will compiler be ok with passing that to a void*?

That should be fine as well, since void* will accept any pointer type, including void**.

#5 Updated by Jason Kraftcheck over 6 years ago

Mark Miller wrote:

We can change now on trunk. We might be able to get away changing ITAPS header file on RC too. However, I don't think we can expect impls to respond prior to 1.2 release.

I have little interest in creating yet another Mesquite point release. I think that in the future we should strongly consider having headers fixed well before we try to stabilize implementations for a release.

And, what happens to any code that is already explicitly casting (void**)? Will compiler be ok with passing that to a void*?

This will not be a problem. Anything (including void**) can be implicitly cast to void*.

I am also a little concerned if when user's see void* as formal argument type, they'll really understand we want a pointer to a pointer.

I agree that this is a concern. I don't feel strongly either way on this issue but for now I'd argue with being consistent with the the rest of the functions by keeping the double pointer. If we really want to make the interface easier to use, I'd vote for someday dropping the offer to have the implementation allocate arrays at all for functions where the caller should already know the necessary array size. That would make the interface much easier to use and this issue would be moot because none of the arrays to tag functions would be passed as double pointers.

#6 Updated by Mark Miller over 6 years ago

I am thinking its possible to change ITAPS header in repo WITHOUT imposing a change on any implementation. That'll mean a default make of unitTest -- which end users are unlikely to attempt to build -- will fail due to this minor spec. difference. However, a make nospecck should pass, right? And, I don't think it means anything anywhere will really break. So, I don't think a new Mesquite release is actually needed in order to accomodate this.

I am fine either way but, like Jason, tend to lean towards NOT changing things at this point. To echo Jason's setiments, we do have bigger API issues than this to worry about anyways.

#7 Updated by Tim Tautges over 6 years ago

I'm fine either way then.

#8 Updated by E. Seegyoung Seol over 6 years ago

Tim Tautges wrote:

I'm fine either way then.

So am I.

#9 Updated by Mark Miller over 6 years ago

  • Target version set to 1.2

#10 Updated by Mark Miller over 6 years ago

  • Assignee set to Mark Miller

#11 Updated by Mark Miller over 6 years ago

Do same for iGeom while we're at it.

#12 Updated by Mark Miller over 6 years ago

  • Status changed from New to Resolved
  • Estimated time set to 0.10

This was changed for headers in ITAPS repo. We acknowledge all products will likely include a slightly out of date header in this regard using void** instead of void*. Hmm, thats probably worst of both worlds at this point as spec. says void* but impl says void** and that'll still require users to explicitly have to cast to void** 'cause the impls are using their own, slightly outdated copies of the spec. header. So, I guess in the end we're not really protecting any users from hitting up against this issue unless we change the impls header files too. We could do that and put copies of all into the release_distros dir for the 1.2 release. However, I am not sure I will feel like doing that.

Also available in: Atom PDF