[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#4044937 <https://reviews.llvm.org/D140860#4044937>, @aaron.ballman wrote:

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

Yeah, I'm OK with that - though I also wouldn't feel strongly about ensuring we warn on the macro case too - if the incremental improvement to do constexpr values is enough for now and a note is left to let someone know they could expand it to handle macros.

But equally it's probably not super difficult to check if the literal is from a macro source location that differs from the source location of either of the operators, I guess? (I guess that check would be needed, so it doesn't warn when the macro is literally 'x && y || true' or the like.

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

Yeah, ideally.


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

https://reviews.llvm.org/D140860