dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land.
In D140860#4047632 <https://reviews.llvm.org/D140860#4047632>, @hazohelet wrote: > In D140860#4047534 <https://reviews.llvm.org/D140860#4047534>, @aaron.ballman > wrote: > >> 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. > > I ran the diagnostic over `microsoft/lightgbm`, `oneapi-src/oneTBB`, and > `rui314/mold` builds. As a result, I found no new warnings from this patch. > > To my surprise, both unpatched/patched clang does not issue the > `-Wlogical-op-parentheses` warning for the following code I mentioned in the > previous comment. > >> https://github.com/oneapi-src/oneTBB/blob/e6e493f96ec8b7e2e2b4d048ed49356eb54ec2a0/src/tbbmalloc/frontend.cpp#L1266 > > It is because clang does not issue warnings on `x || y && z` and `x && y || > z` in the result of macro expansions as of now. > I found an issue on GitHub: > https://github.com/llvm/llvm-project/issues/19345 Ah, thanks for doing the archaeology - and looks like there's an abandoned patch there that might be relevant to addressing some of those macro issues. >> 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. > > Thanks for your help. I think testing macro location against the operator is > already handled in `DiagnoseBinOpPrecedence`, and is somewhat relevant to the > issue above. > > Anyway, I confirm no new instances of parentheses warning in the three > repositories above. Awesome :) Seems reasonable to me, then - how about you, @aaron.ballman ? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140860/new/ https://reviews.llvm.org/D140860 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
