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