[PATCH] D65192: [Sema] Disable some enabled-by-default -Wparentheses diagnostics

[Fangrui Song via Phabricator via cfe-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5654-5656
 def warn_addition_in_bitshift : Warning<
   "operator '%0' has lower precedence than '%1'; "
+  "'%1' will be evaluated first">, InGroup<ShiftOpParentheses>, DefaultIgnore;
----------------
aaron.ballman wrote:
> MaskRay wrote:
> > rsmith wrote:
> > > Do you have evidence that this warning has a significant false-positive rate? In my experience it's very common for people to think of `<<` as being a multiplication-like operator and be surprised when it turns out to have lower precedence than addition.
> > warn_addition_in_bitshift has many false positives. Some results when searching for `[-Wshift-op-parentheses]` and the most common diagnostic `operator '<<' has lower precedence than '+'; '+' will be evaluated first`: 
> > 
> > https://gitship.com/srutscher/pdp-6-emulator/blob/f41b119eee2f409ea519b15f3a76cfecb70c03d8/emu/Makefile
> > https://www.openwall.com/lists/musl/2014/04/04/12
> > https://salmonella-freebsd-x86-64.call-cc.org/chicken-4/clang/freebsd/x86-64/2018/06/21/salmonella-report/install/bvsp-spline.html
> > https://software.intel.com/en-us/forums/intel-threading-building-blocks/topic/431422
> > https://github.com/rdkit/rdkit/issues/145
> > ffmpeg https://bugs.freebsd.org/bugzilla/show_bug.cgi?format=multiple&id=191743
> > https://clang.debian.net/logs/2015-03-25/fairymax_4.8v-1_unstable_clang.log
> > 
> Some of those look like true positives. For instance, the fix to https://github.com/rdkit/rdkit/issues/145 was https://github.com/rdkit/rdkit/commit/38ca41c8abc3f4429ae8df2ad79d3ff8b3fea0b6 which looks like the warning behaved as intended. https://software.intel.com/en-us/forums/intel-threading-building-blocks/topic/431422 doesn't really have anything to do with the diagnostic.
> 
> FWIW, in most of the cases, I feel like parens would clarify the code (heck, they're already using parens in many of these cases). e.g.,
> ```
> fairymax.c:776:46: warning: operator '>>' has lower precedence than '+'; '+' will be evaluated first [-Wshift-op-parentheses]
>                         MovesLeft = -(GamePtr+(Side==WHITE)>>1);
>                                       ~~~~~~~^~~~~~~~~~~~~~~~
> 
> dbvspis.c:606:20: warning: operator '<<' has lower precedence than '+'; '+' will be evaluated first [-Wshift-op-parentheses]
>     i3 = i2 + (*np + 1 << 1);
>                ~~~~^~~ ~~
> ```
> FWIW, I'm fine leaving this default on.
> https://software.intel.com/en-us/forums/intel-threading-building-blocks/topic/431422

Note  -Wno-shift-op-parentheses

> https://github.com/rdkit/rdkit/issues/145 which looks like the warning behaved as intended. 

Yes, this is a true positive. I just randomly searched for some cases, didn't carefully verify them, sorry.

> FWIW, in most of the cases, I feel like parens would clarify the code (heck, they're already using parens in many of these cases). e.g.,

-Wparentheses certainly has its position and it does catch real errors. I also agree that many people are in favor of it. However, there are as many people who dislike it and add -Wno-parentheses to their build systems. Many people complain that extraneous parentheses clutter the code, especially multiple levels of parentheses.

The discrepancy between clang default and gcc default here is a bit unfortunate. People porting software to clang make a lot of style changes. They can mess up git blame, make bugfix backporting difficult, and so on.

The 3 subgroups of -Wparentheses have the most false positives. This is an indicator that they should not belong to the set of default-on warnings. People who use -Wall (a lot) will not notice the difference. I'm thinking if -Wall didn't include these controversial warnings people would be happier (clang -Wmost doesn't include -Wparentheses but unfortunately gcc doesn't have -Wmost). It is also unfortunate -Wparentheses is all-or-nothing...


Repository:
  rC Clang

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

https://reviews.llvm.org/D65192