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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D140860#4033530 <https://reviews.llvm.org/D140860#4033530>, @hazohelet wrote:

> 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.

FWIW a lot of build systems support setting CXXFLAGS/CFLAGS before invoking the build system/build generator (cmake, for instance) and respects those - so might be relatively easy to add a new warning flag to the build (& CXX/CC to set the compiler to point to your local build with the patch/changes)

> 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

So, based on best guess at least from grepping - you don't find any new instances of the warning in these code bases?


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

https://reviews.llvm.org/D140860