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

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jan 11 12:27:21 PST 2023


aaron.ballman added a comment.

In D140860#4031872 <https://reviews.llvm.org/D140860#4031872>, @dblaikie wrote:

> The risk now is that this might significantly regress/add new findings for this warning that may not be sufficiently bug-finding to be worth immediate cleanup, causing users to have to choose between extensive lower-value cleanup and disabling the warning entirely.
>
> Have you/could you run this over a significant codebase to see what sort of new findings the modified warning finds, to see if they're high quality bug finding, or mostly noise/check for whether this starts to detect certain idioms we want to handle differently?
>
> It might be hard to find a candidate codebase that isn't already warning-clean with GCC (at least Clang/LLVM wouldn't be a good candidate because of this) & maybe that's sufficient justification to not worry too much about this outcome...
>
> @aaron.ballman curious what your take on this might be

Thank you for the ping (and the patience waiting on my response)!

I think there's a design here that could make sense to me.

Issuing the diagnostic when there is a literal is silly because the literal value is never going to change. However, with a constant expression, the value could change depending on configuration. This begs the question of: what do we do with literals that are expanded from a macro? It looks like we elide the diagnostic in that case, but macros also imply potential configurability. So I think the design that would make sense to me is to treat macro expansions and constant expressions the same way (diagnose) and only elide the diagnostic when there's a (possibly string) literal. WDYT?

I would also like to see the diagnostic run over some reasonably large corpus of code as it may point out cases where our reasoning is incorrect that we can address. But I don't insist on it in this case given that this is also coming more in line with GCC's behavior (I've not tested how they handle the macro-expanding-to-a-literal case though).


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

https://reviews.llvm.org/D140860



More information about the cfe-commits mailing list