Removed all uses of I as a variable to fix conflict with complex.h#64
Removed all uses of I as a variable to fix conflict with complex.h#64JeffreyBroll wants to merge 7 commits into
Conversation
| * Save individual I values | ||
| */ | ||
| LstarInfo->I[k] = Ifound; | ||
| LstarInfo->I_data[k] = Ifound; |
There was a problem hiding this comment.
Does the structure member also conflict? I.e., is this necessary or is this just to enforce the point?
There was a problem hiding this comment.
I also got complaints with structure members - I didn't think that would be the case...
There was a problem hiding this comment.
I'm still not wild about having to change this since there's a reasonable likelihood of downstream breakage (although I do see the need it just makes me nervous). Does that mean this should prompt a major version update? Or just a caution in release notes and assume that the folks most likely to be affected by this are us...?
There was a problem hiding this comment.
I think this does likely mean a major update as there's an incompatible API change.
If we want to work around this, we could dive further into preprocessor mine-laying by having this split off as a build option - use another placeholder and replace it with I_data if an option like COMPLEX_H_SAFETY is enabled, I if not.
We could also add some of the utilities off of the wishlist(s) so it feels more major.
There was a problem hiding this comment.
Changing structure member names based on build options seems unwise and overly complicated.
I'm all for doing some major updates to justify a major version increment. It's mostly a matter of whether timing works out. Having said that, while it's an incompatible API change I'm not convinced that it's in a place that's generally used for much other than internals. Sure, we use it in the "MagEphem..." utilities, but everything else is basically internal and/or uses K.
I'll give it some thought and take a closer look at what might break (if anything) in my LGM-based codes before getting to merging. Prior to merge this will also need a ChangeLog update, though that update will probably depend on decision re. versioning.
|
I think I got them all. |
drsteve
left a comment
There was a problem hiding this comment.
Working through this, I was concerned that changing the structure members from I to something else (e.g., I_data) might break dependent code.
While we can't trap everything that users may have wanted, we can ensure that we don't break our own tools or examples. I haven't checked the Examples directory, but Tools/MagEphemFromSpiceKernel.c and Tools/MagEphemFromTLE.c are definitely affected. It's likely that the same is true for the MagEphem examples. The Python interface should also be checked... (and it'd be nice to just triple check that the name really needs changing in the structure to avoid the clash).
I'll also need to update some local code that uses I from the structures, so we'll have to make sure this (fairly high chance of breaking things) change is recorded in the changelog to let other users know.
|
I was considering, as an extreme(ly silly) precaution, that we could raise their macros with another macro to re-rename new things back to old... but really I'm just surprised that this collision hasn't become an issue already. I'll start updating and checking examples. |
…hat also needed it along the way
…hat also needed it along the way
Address #62 by removing uses of `I'