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

IlmImf: Fix clang compiler warnings #770

Conversation

arkellr
Copy link
Contributor

@arkellr arkellr commented Jun 24, 2020

Have a review.

With these updates I can compile the repo cleanly with clang (Xcode10/11) with
"-Wall -Wextra -Werror -Wno-unused-parameter -Wno-unused-function"

NB: -Werror = treat all warnings as errors.

Pretty clean now. Could use the above -W flags for the OSX CI.

Fixes:
ImfOpaqueAttribute.h: 'const' type qualifier on return type has no effect [-Werror,-Wignored-qualifiers]
const int dataSize() const { return _dataSize; }

ImfDwaCompressor.cpp: error: comparison of integers of different signs: 'long' and 'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
if (cd->planarUncBufferEnd + dstScanlineSize - _planarUncBuffer[UNKNOWN] > _planarUncBufferSize[UNKNOWN] )

ImfMisc.cpp: warning: unused variable 'maxBytesPerLine' [-Wunused-variable]
size_t maxBytesPerLine = bytesPerLineTable (header,

ImfDeepTiledInputFile.cpp:: error: comparison of integers of different signs: 'Imath_2_5::Int64' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
if(_tileBuffer->dataSize != sizeOfTile)

This allows IlmImf to compile cleanly with clang flags
-Wall -Wextra -Werror -Wno-unused-parameter

Fixes:
ImfOpaqueAttribute.h: 'const' type qualifier on return type has no effect [-Werror,-Wignored-qualifiers]
const int                   dataSize() const { return _dataSize; }
ImfDwaCompressor.cpp: error: comparison of integers of different signs: 'long' and 'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
 if (cd->planarUncBufferEnd + dstScanlineSize - _planarUncBuffer[UNKNOWN] > _planarUncBufferSize[UNKNOWN] )
ImfMisc.cpp: warning: unused variable 'maxBytesPerLine' [-Wunused-variable]
    size_t maxBytesPerLine = bytesPerLineTable (header,
ImfDeepTiledInputFile.cpp:: error: comparison of integers of different signs: 'Imath_2_5::Int64' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
        if(_tileBuffer->dataSize != sizeOfTile)

Signed-off-by: Arkell Rasiah <arasiah@pixsystem.com>
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.

Thanks for addressing these!

OpenEXR/IlmImf/ImfDeepTiledInputFile.cpp Outdated Show resolved Hide resolved
OpenEXR/IlmImf/ImfMisc.cpp Outdated Show resolved Hide resolved
OpenEXR/IlmImf/ImfDwaCompressor.cpp Outdated Show resolved Hide resolved
Update based on review comments.

Signed-off-by: Arkell Rasiah <arasiah@pixsystem.com>
@arkellr arkellr force-pushed the bugfix/arkellr_clang_compiler_warnings branch from 547bf98 to 233cd09 Compare June 24, 2020 19:57
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.

Thanks! Looking good ~ one last thing - there's a std type for doing bitwise math on pointers - with that fix we should be good :)

OpenEXR/IlmImf/ImfDwaCompressor.cpp Outdated Show resolved Hide resolved
Addressing reviewer comment.

Signed-off-by: Arkell Rasiah <arasiah@pixsystem.com>
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.

thanks!!

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.

LGTM

@cary-ilm cary-ilm merged commit a8ac0e0 into AcademySoftwareFoundation:master Jul 8, 2020
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.

None yet

3 participants