Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not warn if half.h has already being included #255

Merged
merged 1 commit into from
May 16, 2022
Merged

Do not warn if half.h has already being included #255

merged 1 commit into from
May 16, 2022

Conversation

jmaselbas
Copy link
Contributor

The issue with the warning of halfLimits.h being deprecated is that it
is very tempting to simply remove the include of halfLimits.h in favor
of simply half.h. However this change does depends on the version of
Imath used, and halfLimits.h should be kept for compatibility with
older version. Simply do not warn if short.h is already included.

The issue with the warning of halfLimits.h being deprecated is that it
is very tempting to simply remove the include of halfLimits.h in favor
of simply half.h. However this change does depends on the version of
Imath used, and halfLimits.h should be kept for compatibility with
older version. Simply do not warn if short.h is already included.

Signed-off-by: Jules Maselbas <[email protected]>
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 30, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: jmaselbas / name: Jules Maselbas (c304ed2)

@lgritz
Copy link
Contributor

lgritz commented Apr 30, 2022

I'm ok with this, but I think at some point we really do want to warn about using the halfLimits.h header, which we would eventually like to remove entirely if it serves no purpose. We wouldn't want to remove headers except at break points when back compatibility can be fully broken (that would be 4.0, by convention), so to give time for people to get the warnings and adjust, there should be a 3.x in which we do want warnings to show up. Maybe it's not 3.1, per this PR. But at some point.

@cary-ilm
Copy link
Member

cary-ilm commented May 2, 2022

Thanks, this looks good to me, and it's indeed a better solution when needing to support older versions of Imath/OpenEXR. The ASWF policy is to require signed Contributor License Agreements for all contributions, no matter how small. If you'd prefer to avoid the hassle of signing the form via the big red button above, I'm happy to submit a similar PR myself, let me know.

@alvinhochun
Copy link

Friendly ping, the author signed the CLA.

I also commented on #241 (comment). I am not entirely against letting users know the include is now "redundant", but I think Imath may at least give an example on how to correctly exclude the include only on supported versions. Having a way to suppress the warning (which is what this PR does) will also be nice.

@cary-ilm
Copy link
Member

Agreed, I filed #258 as a reminder to straighten this out before the next release.

@cary-ilm cary-ilm merged commit d20c3a2 into AcademySoftwareFoundation:main May 16, 2022
cary-ilm pushed a commit to cary-ilm/Imath that referenced this pull request Oct 31, 2022
…dation#255)

The issue with the warning of halfLimits.h being deprecated is that it
is very tempting to simply remove the include of halfLimits.h in favor
of simply half.h. However this change does depends on the version of
Imath used, and halfLimits.h should be kept for compatibility with
older version. Simply do not warn if short.h is already included.

Signed-off-by: Jules Maselbas <[email protected]>
cary-ilm pushed a commit that referenced this pull request Nov 3, 2022
The issue with the warning of halfLimits.h being deprecated is that it
is very tempting to simply remove the include of halfLimits.h in favor
of simply half.h. However this change does depends on the version of
Imath used, and halfLimits.h should be kept for compatibility with
older version. Simply do not warn if short.h is already included.

Signed-off-by: Jules Maselbas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants