[th/preserve-wparentheses-warn] add _c_boolean_expr_() to preseve -Wparentheses warning #11
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
btw, the reason why The downside of the patch is that |
btw, in NetworkManager static-assert is implemented via something like |
thom311
force-pushed
the
th/preserve-wparentheses-warn
branch
2 times, most recently
from
361f775 to
d9e191c
Compare
documentation check complains about indentation. It's not clear to me how to fix that. Various adjustments didn't make a difference. |
Aside the obvious (albeit questionable) benefit of giving the compiler a hint what to expect, _c_likely_() ensures that a "-Wparentheses" warning is triggered with suspicious code like `c_assert(x = 1);`. Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: David Rheinsberg <david.rheinsberg@gmail.com>
Inspired by glib's G_BOOLEAN_EXPR() macro and NetworkManager's NM_BOOLEAN_EXPR() macor. Signed-off-by: Thomas Haller <thaller@redhat.com> (adjust to internal coding-style) Signed-off-by: David Rheinsberg <david.rheinsberg@gmail.com>
This is useful to not hide "-Wparentheses" warnings in case of wrongly
assigning a value like `if (_c_likely_(x = 1)) {`.
Signed-off-by: Thomas Haller <thaller@redhat.com>
Signed-off-by: David Rheinsberg <david.rheinsberg@gmail.com>
|
I did some slight adjustments to the This passes CI and looks ok to me. If you think this is suitable, I will gladly push this! Btw., you have to be careful when using cpp-conditionals for public symbols. The docbook-style comment will assume any block following it is the symbol to document. That's why I always have the |
You reordered the commits, so the commit message of the first commit talks about something that only comes later. Otherwise, looks good to me. Thanks for the cleanup! |
thom311
force-pushed
the
th/preserve-wparentheses-warn
branch
from
d9e191c to
789c4fb
Compare
|
Thanks a lot! Merged! |
|
sigh of course it broke something... Now we get a compilation error with With clang-15.0.4-1.fc37, we get in NetworkManager: here: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/jobs/32667301 Maybe I think the best "solution" is to accept the warning and instead "fix" the unintialized variables to not rely on the compiler to detect @dvdhrm, WDYT? |
would work, but it requires a GCC check, which seems ugly. Btw, what is the purpose of the EDIT: better is which also means that a |
thom311
added a commit
to NetworkManager/c-stdaux
that referenced
this pull request
Nov 29, 2022
We now use _c_likely_() inside c_assert(), and _c_likely_() uses _c_boolean_expr_(). The latter is no longer obviously a compile time constant, and the compiler cannot clearly see that c_assert(0) is an unreachable code path. This means for example, if you have a switch statement and the default case contains "c_assert(0)", then the compiler will now think that this code path can be taken. If you then afterwards access a variable that is initialized in all other switch branches, you get a "-Wsometimes-uninitialized" warning. This happens with clang-15.0.4-1.fc37 and n-dhcp4. See [1]. Work around that by using __builtin_constant_p() in _c_likely_()/ _c_unlikely_(). As we are already inside a C_COMPILER_GNUC check, that is easy to do. This also has the added benefit that now _c_likely_() is a compile time constant, if the argument is. While that is not directly useful, you might get into that situation when using a macro that uses _c_likely_(), like with c_assert(). [1] c-util#11 (comment) Signed-off-by: Thomas Haller <thaller@redhat.com>
|
let's continue at #12 :) |
thom311
added a commit
to NetworkManager/c-stdaux
that referenced
this pull request
Nov 30, 2022
…ly_()/_c_unlikely_() Since recently we use _c_likely_() and _c_boolean_expr_() inside c_assert(). As _c_boolean_expr_() is merely an statement expression, the compiler cannot recognize constants. This is for example a problem with c_assert(0) to indicate unreachable code. With the statement expression, the compiler no longer sees that the code always fails an assertion (as long as NDEBUG is undefined). That has consequences for code that follows the statement, which we know is unreachable. It may lead to compiler warnings like "-Wsometimes-uninitialized". Fix that, by evaluating constant expressions explicitly. With this change, _c_boolean_expr_() can also be used in places where a constant is required, like `char buffer[_c_boolean_expr_(true) ? 5 : 10];`. Of course, this is not a useful thing to do directly, but you might use a macro that ends up effectively calling _c_boolean_expr_() in a context where a constant is required. There is also a unit test that causes a "-Wsometimes-uninitialized" warning with clang-15.0.4-1.fc37. [1] c-util#11 (comment) Signed-off-by: Thomas Haller <thaller@redhat.com>
Debatable? ;) The macro should have the same behavior as This just seemed like the safer option than shortcutting it ourselves. For evetrything else: yeah, lets continue in #12. |
thom311
added a commit
to NetworkManager/c-stdaux
that referenced
this pull request
Nov 30, 2022
…ly_()/_c_unlikely_() Since recently we use _c_likely_() and _c_boolean_expr_() inside c_assert(). As _c_boolean_expr_() is merely an statement expression, the compiler cannot recognize constants. This is for example a problem with c_assert(0) to indicate unreachable code. With the statement expression, the compiler no longer sees that the code always fails an assertion (as long as NDEBUG is undefined). That has consequences for code that follows the statement, which we know is unreachable. It may lead to compiler warnings like "-Wsometimes-uninitialized". Fix that, by evaluating constant expressions explicitly. With this change, _c_boolean_expr_() can also be used in places where a constant is required, like `char buffer[_c_boolean_expr_(true) ? 5 : 10];`. Of course, this is not a useful thing to do directly, but you might use a macro that ends up effectively calling _c_boolean_expr_() in a context where a constant is required. There is also a unit test that causes a "-Wsometimes-uninitialized" warning with clang-15.0.4-1.fc37. [1] c-util#11 (comment) Signed-off-by: Thomas Haller <thaller@redhat.com>
thom311
added a commit
to NetworkManager/c-stdaux
that referenced
this pull request
Nov 30, 2022
…ly_()/_c_unlikely_() Since recently we use _c_likely_() and _c_boolean_expr_() inside c_assert(). As _c_boolean_expr_() is merely an statement expression, the compiler cannot recognize constants. This is for example a problem with c_assert(0) to indicate unreachable code. With the statement expression, the compiler no longer sees that the code always fails an assertion (as long as NDEBUG is undefined). That has consequences for code that follows the statement, which we know is unreachable. It may lead to compiler warnings like "-Wsometimes-uninitialized". Fix that, by evaluating constant expressions explicitly. With this change, _c_boolean_expr_() can also be used in places where a constant is required, like `char buffer[_c_boolean_expr_(true) ? 5 : 10];`. Of course, this is not a useful thing to do directly, but you might use a macro that ends up effectively calling _c_boolean_expr_() in a context where a constant is required. There is also a unit test that causes a "-Wsometimes-uninitialized" warning with clang-15.0.4-1.fc37. [1] c-util#11 (comment) Signed-off-by: Thomas Haller <thaller@redhat.com>
thom311
added a commit
to NetworkManager/c-stdaux
that referenced
this pull request
Nov 30, 2022
…ly_()/_c_unlikely_() Since recently we use _c_likely_() and _c_boolean_expr_() inside c_assert(). As _c_boolean_expr_() is merely an statement expression, the compiler cannot recognize constants. This is for example a problem with c_assert(0) to indicate unreachable code. With the statement expression, the compiler no longer sees that the code always fails an assertion (as long as NDEBUG is undefined). That has consequences for code that follows the statement, which we know is unreachable. It may lead to compiler warnings like "-Wsometimes-uninitialized". Fix that, by evaluating constant expressions explicitly. With this change, _c_boolean_expr_() can also be used in places where a constant is required, like `char buffer[_c_boolean_expr_(true) ? 5 : 10];`. Of course, this is not a useful thing to do directly, but you might use a macro that ends up effectively calling _c_boolean_expr_() in a context where a constant is required. There is also a unit test that causes a "-Wsometimes-uninitialized" warning with clang-15.0.4-1.fc37. [1] c-util#11 (comment) Signed-off-by: Thomas Haller <thaller@redhat.com>
thom311
added a commit
to NetworkManager/c-stdaux
that referenced
this pull request
Nov 30, 2022
…ly_()/_c_unlikely_() Since recently we use _c_likely_() and _c_boolean_expr_() inside c_assert(). As _c_boolean_expr_() is merely an statement expression, the compiler cannot recognize constants. This is for example a problem with c_assert(0) to indicate unreachable code. With the statement expression, the compiler no longer sees that the code always fails an assertion (as long as NDEBUG is undefined). That has consequences for code that follows the statement, which we know is unreachable. It may lead to compiler warnings like "-Wsometimes-uninitialized". Fix that, by evaluating constant expressions explicitly. With this change, _c_boolean_expr_() can also be used in places where a constant is required, like `char buffer[_c_boolean_expr_(true) ? 5 : 10];`. Of course, this is not a useful thing to do directly, but you might use a macro that ends up effectively calling _c_boolean_expr_() in a context where a constant is required. There is also a unit test that causes a "-Wsometimes-uninitialized" warning with clang-15.0.4-1.fc37. [1] c-util#11 (comment) Signed-off-by: Thomas Haller <thaller@redhat.com>
thom311
added a commit
to NetworkManager/c-stdaux
that referenced
this pull request
Nov 30, 2022
…ly_()/_c_unlikely_() Since recently we use _c_likely_() and _c_boolean_expr_() inside c_assert(). As _c_boolean_expr_() is merely an statement expression, the compiler cannot recognize constants. This is for example a problem with c_assert(0) to indicate unreachable code. With the statement expression, the compiler no longer sees that the code always fails an assertion (as long as NDEBUG is undefined). That has consequences for code that follows the statement, which we know is unreachable. It may lead to compiler warnings like "-Wsometimes-uninitialized". Fix that, by evaluating constant expressions explicitly. With this change, _c_boolean_expr_() can also be used in places where a constant is required, like `char buffer[_c_boolean_expr_(true) ? 5 : 10];`. Of course, this is not a useful thing to do directly, but you might use a macro that ends up effectively calling _c_boolean_expr_() in a context where a constant is required (like c_assert() using _c_likely_() and _c_boolean_expr_()). There is also a unit test that would cause a "-Wsometimes-uninitialized" warning with clang-15.0.4-1.fc37. Note that __builtin_choose_expr() is only used with GCC greater than 4. That's because __builtin_constant_p() inside __builtin_choose_expr() does not properly work with gcc 4.8 ([2]). It's not too bad. First of all, c-stdaux is a C11 library, while gcc 4 has only partial C11 support to being with (it still works in large parts though). Also, the entire point of _c_boolean_expr_() is to preserve the "-Wparentheses" warning for a macro argument (while being function-like, evaluating arguments once, use a __COUNTER__ name, and propagating the constness with __builtin_choose_expr). It's almost the same as "(!!(_x))" were it not for "-Wparentheses". We can afford to miss that on non-GCC or on GCC not newer than 4. [1] c-util#11 (comment) [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449 Signed-off-by: Thomas Haller <thaller@redhat.com>
dvdhrm
pushed a commit
to dvdhrm/c-stdaux
that referenced
this pull request
Dec 15, 2022
…ly_()/_c_unlikely_() Since recently we use _c_likely_() and _c_boolean_expr_() inside c_assert(). As _c_boolean_expr_() is merely an statement expression, the compiler cannot recognize constants. This is for example a problem with c_assert(0) to indicate unreachable code. With the statement expression, the compiler no longer sees that the code always fails an assertion (as long as NDEBUG is undefined). That has consequences for code that follows the statement, which we know is unreachable. It may lead to compiler warnings like "-Wsometimes-uninitialized". Fix that, by evaluating constant expressions explicitly. With this change, _c_boolean_expr_() can also be used in places where a constant is required, like `char buffer[_c_boolean_expr_(true) ? 5 : 10];`. Of course, this is not a useful thing to do directly, but you might use a macro that ends up effectively calling _c_boolean_expr_() in a context where a constant is required (like c_assert() using _c_likely_() and _c_boolean_expr_()). There is also a unit test that would cause a "-Wsometimes-uninitialized" warning with clang-15.0.4-1.fc37. Note that __builtin_choose_expr() is only used with GCC greater than 4. That's because __builtin_constant_p() inside __builtin_choose_expr() does not properly work with gcc 4.8 ([2]). It's not too bad. First of all, c-stdaux is a C11 library, while gcc 4 has only partial C11 support to being with (it still works in large parts though). Also, the entire point of _c_boolean_expr_() is to preserve the "-Wparentheses" warning for a macro argument (while being function-like, evaluating arguments once, use a __COUNTER__ name, and propagating the constness with __builtin_choose_expr). It's almost the same as "(!!(_x))" were it not for "-Wparentheses". We can afford to miss that on non-GCC or on GCC not newer than 4. [1] c-util#11 (comment) [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449 Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: David Rheinsberg <david.rheinsberg@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
gcc has a useful warning
-Wparentheses, for anti patterns likeif (x =1) ..., which suggests to either use "==" or put the assignment in a parentheses.Inside macros (like
c_assert()) we need to make sure to add proper parentheses around the macro arguments, but if we just always add them, we hide the useful warning.Add
_c_boolean_expr_()which with GCC uses and expression statement and__COUNTER__to evaluate the expression to 0 or 1, without hiding-Wparentheses. And use it from_c_likely_(),_c_unlikely_(),c_assert().This is also done by glib and NetworkManager.