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

Fix compiler warnings on windows #250

Merged

Conversation

fpsunflower
Copy link
Contributor

MSVC is fairly picking about casts that potentially loose precision. This made up the bulk of the warnings that occur out of the box on windows.

None of these are particularly serious and they almost all occur inside unit tests, but hopefully by fixing them it makes it more obvious if there are new warnings introduced in the future.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 8, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: fpsunflower / name: Christopher Kulla (22bc38b)

@fpsunflower fpsunflower marked this pull request as draft April 8, 2022 22:22
@fpsunflower fpsunflower force-pushed the ck-fix-windows-warnings branch 2 times, most recently from 7410acb to a5a32b0 Compare April 8, 2022 22:40
@fpsunflower fpsunflower marked this pull request as ready for review April 8, 2022 23:41
@fpsunflower
Copy link
Contributor Author

Ready for review. I should be able to get the CLA stuff sorted out next week.

Copy link
Contributor

@meshula meshula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great clean up, thanks for taking it on. Question online about static_cast versus constructor.

src/ImathTest/testExtractSHRT.cpp Show resolved Hide resolved
src/Imath/ImathMatrix.h Outdated Show resolved Hide resolved
@xlietz
Copy link
Contributor

xlietz commented Apr 13, 2022

@fpsunflower I added you to the OpenEXR approved list of contributors under Epic's CCLA. Sorry for the delay!

@fpsunflower fpsunflower force-pushed the ck-fix-windows-warnings branch from a5a32b0 to 22bc38b Compare April 15, 2022 21:43
@fpsunflower
Copy link
Contributor Author

@fpsunflower I added you to the OpenEXR approved list of contributors under Epic's CCLA. Sorry for the delay!

Thanks! Do I need to be added for the Imath repo separately? It looks like its still failing the automated check ...

@fpsunflower
Copy link
Contributor Author

Nevermind @xlietz - the CLA seems to be working now! Thanks!

Signed-off-by: Chris Kulla <ckulla@gmail.com>
@fpsunflower fpsunflower force-pushed the ck-fix-windows-warnings branch from 22bc38b to 25983fa Compare April 18, 2022 16:06
Copy link
Member

@cary-ilm cary-ilm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay in getting this merged, it all looks good to me.

@cary-ilm cary-ilm merged commit 3ffb078 into AcademySoftwareFoundation:main May 2, 2022
cary-ilm added a commit to cary-ilm/Imath that referenced this pull request Jun 2, 2022
PR AcademySoftwareFoundation#250 changed the %lx to %llx to suppress an MSVC warning, but it
introduced a gcc warning, where uint64_t is long, not long
long. Better to leave it as %lx and just cast to long. It's only a
printf anyway.

Signed-off-by: Cary Phillips <cary@ilm.com>
cary-ilm added a commit that referenced this pull request Oct 24, 2022
* Fix gcc compiler warning in testFun.cpp

PR #250 changed the %lx to %llx to suppress an MSVC warning, but it
introduced a gcc warning, where uint64_t is long, not long
long. Better to leave it as %lx and just cast to long. It's only a
printf anyway.

Signed-off-by: Cary Phillips <cary@ilm.com>

* cast to unsigned long long for %llx

Signed-off-by: Cary Phillips <cary@ilm.com>

* In printf, bit_cast to uint<n>_t and use PRIx<n>

This shoudl properly avoid warnings on all platforms.

Signed-off-by: Cary Phillips <cary@ilm.com>

Signed-off-by: Cary Phillips <cary@ilm.com>
cary-ilm pushed a commit to cary-ilm/Imath that referenced this pull request Oct 31, 2022
Signed-off-by: Chris Kulla <ckulla@gmail.com>
cary-ilm added a commit to cary-ilm/Imath that referenced this pull request Oct 31, 2022
* Fix gcc compiler warning in testFun.cpp

PR AcademySoftwareFoundation#250 changed the %lx to %llx to suppress an MSVC warning, but it
introduced a gcc warning, where uint64_t is long, not long
long. Better to leave it as %lx and just cast to long. It's only a
printf anyway.

Signed-off-by: Cary Phillips <cary@ilm.com>

* cast to unsigned long long for %llx

Signed-off-by: Cary Phillips <cary@ilm.com>

* In printf, bit_cast to uint<n>_t and use PRIx<n>

This shoudl properly avoid warnings on all platforms.

Signed-off-by: Cary Phillips <cary@ilm.com>

Signed-off-by: Cary Phillips <cary@ilm.com>
cary-ilm pushed a commit that referenced this pull request Nov 3, 2022
Signed-off-by: Chris Kulla <ckulla@gmail.com>
cary-ilm added a commit that referenced this pull request Nov 3, 2022
* Fix gcc compiler warning in testFun.cpp

PR #250 changed the %lx to %llx to suppress an MSVC warning, but it
introduced a gcc warning, where uint64_t is long, not long
long. Better to leave it as %lx and just cast to long. It's only a
printf anyway.

Signed-off-by: Cary Phillips <cary@ilm.com>

* cast to unsigned long long for %llx

Signed-off-by: Cary Phillips <cary@ilm.com>

* In printf, bit_cast to uint<n>_t and use PRIx<n>

This shoudl properly avoid warnings on all platforms.

Signed-off-by: Cary Phillips <cary@ilm.com>

Signed-off-by: Cary Phillips <cary@ilm.com>
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