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

Thu Aug 1 19:25:54 PDT 2019

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:
> > 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...
> > -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.
> 
> They don't seem to be complaining to our bug tracker -- I did not see any complaints about the behavior of `warn_addition_in_bitshift` (there were a few complaints about some of the other issues you're addressing in this patch though).
> 
> My concern is that we have plenty of evidence to demonstrate that users do not enable warnings that are off by default, and this warning does catch true positive issues that can be subtle and hard to track down without the warning. I'm not yet convinced that this has a high enough false-positive warning rate to justify default-offing it, and so my preference is to hold off on this one to see if it really needs to be default off.
> They don't seem to be complaining to our bug tracker -- I did not see any complaints about the behavior of warn_addition_in_bitshift (there were a few complaints about some of the other issues you're addressing in this patch though).

People tend to use -Wno-parentheses, but there are still plenty of -Wno-shift-op-parentheses. I don't agree that we need to be clang bug-report driven. People just don't bother fighting with the numerous compiler versions and adding -Wno-parentheses -Wno-shift-op-parentheses (some projects start from a clean state and add warnings they desire).

Sure I can take a step back and don't change the default state of -Wshift-op-parentheses. (People sometimes use omission of spaces between some operands/operators to hint how the expression groups. Unforunately -Wshift-op-parentheses doesn't know this). 

-Wbitwise-op-parentheses, e.g. i & i | i
-Wlogical-op-parentheses, e.g. i && i || i

are what people hate the most and I do want to see them disabled as soon as possible.

Repository:
  rC Clang

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

https://reviews.llvm.org/D65192