aaron.ballman 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; ---------------- 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. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65192/new/ https://reviews.llvm.org/D65192 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
