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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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

> 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 mostly don't want to insist on dealing with macros in this patch, but it does leave the diagnostic behavior somewhat inconsistent to my mind. I think I can live without the macro functionality though, as this is still forward progress. And yes, you'd need to check the macro location against the operator location, I believe. Testing for a macro expansion is done with `SourceLocation::isMacroID()`, in case @hazohelet wants to try to implement that functionality as well.


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

https://reviews.llvm.org/D140860