Do not define AVIF_ENABLE_NODISCARD for avif.h #2351
Conversation
wantehchang
commented
Jul 31, 2024
•
wantehchang
commented
|
@vrabaud FYI. |
CMakeLists.txt
Outdated
| @@ -33,8 +33,16 @@ include(FetchContent) | |||
| include(FindPkgConfig) | |||
| include(AvifExternalProjectUtils) | |||
|
|
|||
| option(AVIF_ENABLE_NODISCARD "Add [[nodiscard]] to some functions. CMake must be at least 3.21 to request C23." OFF) | |||
There was a problem hiding this comment.
This is copied from line 55 in the original code, with "force C23" changed to "request C23."
| if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.21.0) | ||
| set(CMAKE_C_STANDARD 23) | ||
| set_property(TARGET avif_obj PROPERTY C_STANDARD 23) | ||
| else() |
There was a problem hiding this comment.
If we redefine AVIF_ENABLE_NODISCARD to be a request rather than a requirement, we don't need this else block.
| target_compile_options(avif_obj PUBLIC $<BUILD_INTERFACE:$<$<COMPILE_LANGUAGE:C>:/std:clatest>>) | ||
| endif() | ||
| endif() | ||
| target_compile_definitions(avif_obj PUBLIC $<BUILD_INTERFACE:AVIF_ENABLE_NODISCARD=1>) |
There was a problem hiding this comment.
The definition of the AVIF_ENABLE_NODISCARD macro for avif.h is also deleted.
There was a problem hiding this comment.
This makes sense.
The decaying is unfortunate:
https://cmake.org/cmake/help/latest/manual/cmake-compile-features.7.html#id8
But it looks like CMAKE_C_STANDARD_REQUIRED might work if we wanted to abort when the option was specified, but the version not available.
wantehchang
force-pushed
the
make-AVIF_ENABLE_NODISCARD-best-effort
branch
from
42c32ab
to
d64aa4d
Compare
Redefine the AVIF_ENABLE_NODISCARD cmake option to request rather than require the AVIF_NODISCARD macro be defined as [[nodiscard]]. There are two reasons: 1. The CMAKE_C_STANDARD variable merely requests the given version of the C standard. If the compiler doesn't support the requested version of the C standard, it "decays" to a previous version of the C standard. Making avif.h use [[nodiscard]] in this case will result in compilation errors. 2. In general avif.h should only test compiler predefined macros. The only exception is AVIF_DLL (ignoring the experimental feature macros.)
wantehchang
force-pushed
the
make-AVIF_ENABLE_NODISCARD-best-effort
branch
from
d64aa4d
to
0c616d2
Compare
wantehchang
force-pushed
the
make-AVIF_ENABLE_NODISCARD-best-effort
branch
from
0c616d2
to
35c4a4e
Compare
Thank you for the suggestion. I implemented this in the second commit. I also rewrote the commit message and pasted it at the top of this issue. Please take a look. Thanks! |
| set(CMAKE_C_STANDARD 23) | ||
| set(CMAKE_C_STANDARD_REQUIRED ON) | ||
| else() | ||
| set(CMAKE_C_STANDARD 99) |
There was a problem hiding this comment.
I would think this would be required too, but it's highly unlikely anyone would try this with a c90 only compiler (or force those flags)
There was a problem hiding this comment.
Yes, we assume the compiler supports C99.
