[PATCH] D140860: [Diagnostics][NFC] Fix -Wlogical-op-parentheses warning inconsistency for const and constexpr values

Takuya Shimizu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sat Jan 7 07:47:55 PST 2023


hazohelet added a comment.

I have yet to do thorough checks using this patched clang to build significant code bases.
It will likely take quite a bit of time as I am not used to editing build tool files.

Instead, I used `grep` to find potentially newly-warned codes.
The `grep` command is shown below. It is intended to match C/C++ codes with both `&&` and `||` within which 0, 1, or 2 matching parentheses exist in a single line.
`grep -r --include=\*.cpp --include=\*.cc  --include=\*.c -e "&&[^()]*||" -e "||[^()]*&&" -e "&&[^()]*([^()]*)[^()]*||" -e "||[^()]*([^()]*)[^()]*&&" -e "&&[^()]*([^()]*([^()]*)[^()]*)[^()]*||" -e "||[^()]*([^()]*([^()]*)[^()]*)[^()]*&&"`

I applied this `grep` command to the following popular GitHub repositories `NVIDIA/TensorRT`, `bulletphysics/bullet3`, `apple/foundationdb`, `grpc/grpc`, `microsoft/lightgbm`, `rui314/mold`, `oneapi-src/oneTBB`, `SerenityOS/serenity`, `tensorflow/tensorflow`.
I found the 7 examples of missing parentheses at `&&` within `||` in the matched strings (many of the matchings exist in preprocessor conditionals, which is out of the scope of this patch)
They are listed at the end of this comment.
The last case in tensorflow is an `assert(a || b && "reason for the assert")` known idiom and is handled correctly.
Because the newly-warned constants are compile-time constants not dependent on function arguments, the other 6 cases will also get warnings from the clang before this patch.

It suggests that this patch does not generate extensive new warnings in existent codebases.

oneTBB:
https://github.com/oneapi-src/oneTBB/blob/e6e493f96ec8b7e2e2b4d048ed49356eb54ec2a0/examples/common/gui/xvideo.cpp#L359
https://github.com/oneapi-src/oneTBB/blob/e6e493f96ec8b7e2e2b4d048ed49356eb54ec2a0/src/tbbmalloc/frontend.cpp#L1266
tensorflow:
https://github.com/tensorflow/tensorflow/blob/fb2c3b1e3140af73f949981d8428379cbb28228b/tensorflow/compiler/tf2tensorrt/convert/convert_nodes_test.cc#L4130
https://github.com/tensorflow/tensorflow/blob/fb2c3b1e3140af73f949981d8428379cbb28228b/tensorflow/compiler/tf2tensorrt/convert/convert_nodes_test.cc#L4074
https://github.com/tensorflow/tensorflow/blob/fb2c3b1e3140af73f949981d8428379cbb28228b/tensorflow/compiler/tf2tensorrt/convert/convert_nodes_test.cc#L9456
https://github.com/tensorflow/tensorflow/blob/fb2c3b1e3140af73f949981d8428379cbb28228b/tensorflow/compiler/jit/deadness_analysis.cc#L1423
https://github.com/tensorflow/tensorflow/blob/fb2c3b1e3140af73f949981d8428379cbb28228b/tensorflow/core/ir/tf_op_wrapper.cc#L25


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140860/new/

https://reviews.llvm.org/D140860



More information about the cfe-commits mailing list