[PATCH] D66397: [Diagnostics] Improve -Wxor-used-as-pow

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

> I don't understand how you draw that conclusion, but the reason behind why we went with that as a way to silence the diagnostic is because using the prefix acts as a signal that the developer wants to do bit manipulation more than just a decimal literal does. It's not ideal because it's largely a hidden way to silence diagnostics, but it's also not the only place where we do that sort of thing (like surrounding something in parens to silence a diagnostic, or casting something to void, etc).

I think adding parens and casts are fairly well-understood to suppress warnings. Other things, like changing the spelling of literals, aren't. C++ is already a fairly baroque language, and every new thing that makes your code behave subtly different with a different compiler makes it more so. So I think we should be careful to not attach semantic meaning in surprising ways. Warnings have to carry their weight.

>> Maybe a better fixit is to suggest `xor` instead of `^` which also suppresses the warning and which is imho a bit less weird. (`xor` is so rare that it's still a bit weird though.)
> 
> I don't think suggesting a fixit for `xor` is a good solution. For starters, you need a header included in order to use that for C. As a textual suggestion "; use the 'xor' alternative token to perform an exclusive OR" wouldn't bother me too much though.

I don't like xor all that much either :)

I do like Richard's suggestion.

Quuxplusone: Saying "the chromium devs don't want to change their code" is missing the mark. I'm happy to change that file, but that doesn't really help. There are hundreds of people committing hundreds of changes to the code base every day. They have varying levels of C++ proficiency (like everywhere). It's not just about getting our code to compile today, but also about the long-term effects of the warnings we have enabled. We have to be careful about which warnings we enable, and this one is maybe not quite there yet on a usefulness/overhead tradeoff evaluation. I'd argue that Chromium is fairly typical here and that clang's default warning set should follow similar considerations.

I looked through D63423 <https://reviews.llvm.org/D63423> and didn't find an evaluation of false / true positive rates when lhs and rhs are just literals vs when they're identifiers / macros. Glancing through https://codesearch.isocpp.org/cgi-bin/cgi_ppsearch?q=10+%5E&search=Search , I couldn't find a single true positive where lhs or rhs weren't a bare literal (but I didn't look super carefully). Did I just miss that in D63423 <https://reviews.llvm.org/D63423>? What was the motivation for firing on more than bare literals?


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

https://reviews.llvm.org/D66397