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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
