Make C++ compilation strict, treat warnings as errors #452
Conversation
facebook-github-bot
added
the
CLA Signed
scotts
changed the title
| @@ -21,7 +21,7 @@ int64_t getDuration(const UniqueAVFrame& frame) { | |||
| } | |||
|
|
|||
| int64_t getDuration(const AVFrame* frame) { | |||
| #if LIBAVUTIL_VERSION_MAJOR < 59 | |||
| #if LIBAVUTIL_VERSION_MAJOR < 58 | |||
There was a problem hiding this comment.
I think we just had this wrong. It was deprecated in 58.
| @@ -206,7 +206,8 @@ VideoDecoder::BatchDecodedOutput::BatchDecodedOutput( | |||
|
|
|||
| bool VideoDecoder::DecodedFrameContext::operator==( | |||
| const VideoDecoder::DecodedFrameContext& other) { | |||
| return decodedWidth == other.decodedWidth && decodedHeight == decodedHeight && | |||
There was a problem hiding this comment.
Note that the stricter compilation revealed a real logic error here, as we had the code decodedHeight == decodedHeight.
| void VideoDecoder::validateUserProvidedStreamIndex(uint64_t streamIndex) { | ||
| size_t streamsSize = containerMetadata_.streams.size(); | ||
| void VideoDecoder::validateUserProvidedStreamIndex(int streamIndex) { | ||
| int streamsSize = static_cast<int>(containerMetadata_.streams.size()); | ||
| TORCH_CHECK( | ||
| streamIndex >= 0 && streamIndex < streamsSize, |
There was a problem hiding this comment.
Because we had declared streamIndex as uint64_t, this check for streamIndex >= 0 was trivially always true.
There was a problem hiding this comment.
Thanks for the fixes @scotts ! Looks like we have some compile failures. Not sure if those are from this PR or if main is failing too - do you mind checking?
| @@ -2,6 +2,7 @@ cmake_minimum_required(VERSION 3.18) | |||
| project(TorchCodec) | |||
| set(CMAKE_CXX_STANDARD 17) | |||
| set(CMAKE_CXX_STANDARD_REQUIRED ON) | |||
| add_compile_options(-Wall -Wextra -pedantic -Werror) | |||
There was a problem hiding this comment.
I did a non-exhaustive google search and it looks like add_compile_options is similar to setting CMAKE_CXX_FLAGS (as we do below line 7-8), with a different scope.
While some sources suggest that add_compile_options is better scoped, I feel like we should stick to setting CMAKE_CXX_FLAGS here. Because a) this is what we already do, so it's best to only have one way to set flags, and b) we want to set the flags of the entire project anyway, for all targets, which is what CMAKE_CXX_FLAGS does.
|
The compilation failures are almost certainly caused by this PR; I'll dig in. What this does open us up to is slight differences in compilers can now cause compilation failure. For example, everything compiled clean on my system, but then I've had to deal with at least two problems in CI. On the one hand, that's kind of the point of this PR: let's be more strict with the hopes we will fail less internally. On the other hand, it does mean we (and other open source devs) will run into this whack-a-mole with CI systems may be more strict than local. Hopefully that calms down after this PR that changes a bunch at once. |
| @@ -112,7 +112,7 @@ jobs: | |||
| run: | | |||
| cd docs | |||
| ${CONDA_RUN} make html | |||
| - uses: actions/upload-artifact@v3 | |||
| - uses: actions/upload-artifact@v4 | |||
There was a problem hiding this comment.
Missed doing this one last week, and it only just now started failing.
|
Everything works now. And I have more confidence this is the right thing to do, because the failing checks were in CUDA code. My dev environment is non-CUDA, hence why I didn't catch it locally. |
| @@ -4,7 +4,7 @@ set(CMAKE_CXX_STANDARD 17) | |||
| set(CMAKE_CXX_STANDARD_REQUIRED ON) | |||
|
|
|||
| find_package(Torch REQUIRED) | |||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${TORCH_CXX_FLAGS}") | |||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${TORCH_CXX_FLAGS} -Wall -Wextra -pedantic -Werror") | |||
There was a problem hiding this comment.
Nit: I wonder if we should add those before ${TORCH_CXX_FLAGS} so that they can take precedence?
We still sometimes get C++ code that compiles externally that does not compile internally. I think one way to make this less likely is for our CMake builds to be more restrictive. This means compiling with:
However, turning that on means we need to fixup a bunch of integer comparisons, so there are a lot of small changes in this PR. It's also worth noting that I still could not reproduce the errors I get on internal builds. I think there will always be the possibility of disagreement because of different compilers.