[Sema] Make -Wbitwise-op-parentheses and -Wlogical-op-parentheses disabled-by-default

[MaskRay]

Summary

The -Wparentheses warnings are enabled by default in clang but they are under
-Wall in gcc (gcc/c-family/c.opt). Some of the operator precedence warnings are
oftentimes criticized as noise (clang: default; gcc: -Wall). If a warning is
very controversial, it is probably not a good idea to enable it by default.
This patch disables the rather annoying ones:

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

After this change:

* = enabled by default

-Wall
  -Wparentheses
    -Wlogical-op-parentheses
    -Wlogical-not-parentheses*
    -Wbitwise-op-parentheses
    -Wshift-op-parentheses*
    -Woverloaded-shift-op-parentheses*
    -Wparentheses-equality*
    -Wdangling-else*

-Woverloaded-shift-op-parentheses is typically followed by overload
resolution failure. We can instead improve the error message, and
probably delete -Woverloaded-shift-op-parentheses in the future. Keep it
for now because it gives some diagnostics.

[MaskRay]

Delete -Wmost from the hierarchy. It doesn't belong there

[jyknight]

I'm not sure the history behind why these were added as default-on warnings....they don't really seem appropriate as default warnings to me, either.

But I think someone else probably ought to approve the change.

[rsmith]

For the && vs || and & vs | warnings, it seems extremely implausible to me that they meet the "few or no false-positives" criterion for an enabled-by-default warning. Even assuming that people don't know the relative precedences of those operators, we'd expect a false-positive rate of at least 50%. So disabling those by default seems reasonable to me, and in line with prior guidance for what makes a good on-by-default warning.

For the other two changes, I'm not yet convinced we should change the default, but could be convinced by data about false positive rates.

include/clang/Basic/DiagnosticSemaKinds.td

5647–5650 ↗

(On Diff #211425)

I think this should remain enabled by default unless you have evidence of false positives. In my experience, this catches bugs like

ostream << "got expected result: " << x == 0;

... and very little else.

That said, perhaps we could do better here, since this warning is typically followed by an error: if we detect the special case of overload resolution failure for an operator with an << (or >>) operator expression on its left-hand side, we could produce a much more targeted diagnostic for this case and avoid the current situation of a warning followed by an error for the same problem. If we did that, we could probably remove this warning entirely.

5654–5656 ↗

(On Diff #211425)

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.

[MaskRay]

include/clang/Basic/DiagnosticSemaKinds.td

5654–5656 ↗

(On Diff #211425)

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

[MaskRay]

keep -Woverloaded-shift-op-parentheses enabled by default

[MaskRay]

include/clang/Basic/DiagnosticSemaKinds.td

5647–5650 ↗

(On Diff #211425)

Restored this one.

Searching for Wno-parentheses can probably give more interesting results...

[MaskRay]

Ping :)

[aaron.ballman]

include/clang/Basic/DiagnosticSemaKinds.td

5654–5656 ↗

(On Diff #211425)

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.

[MaskRay]

include/clang/Basic/DiagnosticSemaKinds.td

5654–5656 ↗

(On Diff #211425)

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...

[aaron.ballman]

include/clang/Basic/DiagnosticSemaKinds.td

5654–5656 ↗

(On Diff #211425)

-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.

[MaskRay]

include/clang/Basic/DiagnosticSemaKinds.td

5654–5656 ↗

(On Diff #211425)

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.

[rsmith]

Can you reduce this patch to only the && within || and & within | changes? I think we have reasonable consensus that that should not be enabled by default, so let's land that part now. If you want to continue discussing -Wshift-op-parentheses after that, we can.

[MaskRay]

Retitle

disable just -Wbitwise-op-parentheses and -Wlogical-op-parentheses

[MaskRay]

test/Parser/cxx2a-spaceship.cpp does not need a change

[aaron.ballman]

Thank you for this! LGTM aside from a testing request (doesn't require further review).

include/clang/Basic/DiagnosticSemaKinds.td

5645 ↗

(On Diff #212962)

Can you add a test case for this, please?

[MaskRay]

Add bitwise-op-parentheses.c logical-op-parentheses.c

[MaskRay]

.

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

def warn_logical_instead_of_bitwise : Warning<
   "use of logical '%0' with constant operand">,
   InGroup<DiagGroup<"constant-logical-operand">>;
 def note_logical_instead_of_bitwise_change_operator : Note<
   "use '%0' for a bitwise operation">;
 def note_logical_instead_of_bitwise_remove_constant : Note<
   "remove constant to silence this warning">;
 
 def warn_bitwise_op_in_bitwise_op : Warning<
-  "'%0' within '%1'">, InGroup<BitwiseOpParentheses>;
+  "'%0' within '%1'">, InGroup<BitwiseOpParentheses>, DefaultIgnore;
 
 def warn_logical_and_in_logical_or : Warning<
-  "'&&' within '||'">, InGroup<LogicalOpParentheses>;
+  "'&&' within '||'">, InGroup<LogicalOpParentheses>, DefaultIgnore;
 
 def warn_overloaded_shift_in_comparison :Warning<
   "overloaded operator %select{>>|<<}0 has higher precedence than "
   "comparison operator">,
   InGroup<OverloadedShiftOpParentheses>;
 def note_evaluate_comparison_first :Note<
   "place parentheses around comparison expression to evaluate it first">;
 

cfe/trunk/test/Sema/bitwise-op-parentheses.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s -DSILENCE
+// RUN: %clang_cc1 -fsyntax-only -verify %s -Wbitwise-op-parentheses
+// RUN: %clang_cc1 -fsyntax-only -verify %s -Wparentheses
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s -Wbitwise-op-parentheses 2>&1 | FileCheck %s
+
+#ifdef SILENCE
+// expected-no-diagnostics
+#endif
+
+void bitwise_op_parentheses(unsigned i) {
+  (void)(i & i | i);
+#ifndef SILENCE
+  // expected-warning@-2 {{'&' within '|'}}
+  // expected-note@-3 {{place parentheses around the '&' expression to silence this warning}}
+#endif
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:10-[[@LINE-5]]:10}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:15-[[@LINE-6]]:15}:")"
+
+  (void)(i | i & i);
+#ifndef SILENCE
+  // expected-warning@-2 {{'&' within '|'}}
+  // expected-note@-3 {{place parentheses around the '&' expression to silence this warning}}
+#endif
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:14-[[@LINE-5]]:14}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:19-[[@LINE-6]]:19}:")"
+
+  (void)(i ^ i | i);
+#ifndef SILENCE
+  // expected-warning@-2 {{'^' within '|'}}
+  // expected-note@-3 {{place parentheses around the '^' expression to silence this warning}}
+#endif
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:10-[[@LINE-5]]:10}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:15-[[@LINE-6]]:15}:")"
+
+  (void)(i | i ^ i);
+#ifndef SILENCE
+  // expected-warning@-2 {{'^' within '|'}}
+  // expected-note@-3 {{place parentheses around the '^' expression to silence this warning}}
+#endif
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:14-[[@LINE-5]]:14}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:19-[[@LINE-6]]:19}:")"
+
+  (void)(i & i ^ i);
+#ifndef SILENCE
+  // expected-warning@-2 {{'&' within '^'}}
+  // expected-note@-3 {{place parentheses around the '&' expression to silence this warning}}
+#endif
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:10-[[@LINE-5]]:10}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:15-[[@LINE-6]]:15}:")"
+
+  (void)(i ^ i & i);
+#ifndef SILENCE
+  // expected-warning@-2 {{'&' within '^'}}
+  // expected-note@-3 {{place parentheses around the '&' expression to silence this warning}}
+#endif
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:14-[[@LINE-5]]:14}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:19-[[@LINE-6]]:19}:")"
+}

cfe/trunk/test/Sema/logical-op-parentheses.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s -DSILENCE
+// RUN: %clang_cc1 -fsyntax-only -verify %s -Wlogical-op-parentheses
+// RUN: %clang_cc1 -fsyntax-only -verify %s -Wparentheses
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s -Wlogical-op-parentheses 2>&1 | FileCheck %s
+
+#ifdef SILENCE
+// expected-no-diagnostics
+#endif
+
+void logical_op_parentheses(unsigned i) {
+  (void)(i ||
+             i && i);
+#ifndef SILENCE
+  // expected-warning@-2 {{'&&' within '||'}}
+  // expected-note@-3 {{place parentheses around the '&&' expression to silence this warning}}
+#endif
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:14-[[@LINE-5]]:14}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:20-[[@LINE-6]]:20}:")"
+
+  (void)(i || i && "w00t");
+  (void)("w00t" && i || i);
+
+  (void)(i || i && "w00t" || i);
+#ifndef SILENCE
+  // expected-warning@-2 {{'&&' within '||'}}
+  // expected-note@-3 {{place parentheses around the '&&' expression to silence this warning}}
+#endif
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:15-[[@LINE-5]]:15}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:26-[[@LINE-6]]:26}:")"
+
+  (void)(i || "w00t" && i || i);
+#ifndef SILENCE
+  // expected-warning@-2 {{'&&' within '||'}}
+  // expected-note@-3 {{place parentheses around the '&&' expression to silence this warning}}
+#endif
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:15-[[@LINE-5]]:15}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:26-[[@LINE-6]]:26}:")"
+
+  (void)(i && i || 0);
+  (void)(0 || i && i);
+}

cfe/trunk/test/Sema/parentheses.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s
 // RUN: %clang_cc1 -Wparentheses -fsyntax-only -verify %s
 // RUN: %clang_cc1 -Wparentheses -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 // Test the various warnings under -Wparentheses
 void if_assign(void) {
   int i;
   if (i = 4) {} // expected-warning {{assignment as a condition}} \
                 // expected-note{{place parentheses around the assignment to silence this warning}} \
void bitwise_rel(unsigned i) {
   // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:10-[[@LINE-5]]:10}:"("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:18-[[@LINE-6]]:18}:")"
 
   (void)((i & 0x2) == 0);
   (void)(i & (0x2 == 0));
   // Eager logical op
   (void)(i == 1 | i == 2 | i == 3);
   (void)(i != 1 & i != 2 & i != 3);
-
-  (void)(i & i | i); // expected-warning {{'&' within '|'}} \
-                     // expected-note {{place parentheses around the '&' expression to silence this warning}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
-
-  (void)(i | i & i); // expected-warning {{'&' within '|'}} \
-                     // expected-note {{place parentheses around the '&' expression to silence this warning}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:14-[[@LINE-2]]:14}:"("
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:19-[[@LINE-3]]:19}:")"
-
-  (void)(i ^ i | i); // expected-warning {{'^' within '|'}} \
-                     // expected-note {{place parentheses around the '^' expression to silence this warning}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
-
-  (void)(i | i ^ i); // expected-warning {{'^' within '|'}} \
-                     // expected-note {{place parentheses around the '^' expression to silence this warning}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:14-[[@LINE-2]]:14}:"("
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:19-[[@LINE-3]]:19}:")"
-
-  (void)(i & i ^ i); // expected-warning {{'&' within '^'}} \
-                     // expected-note {{place parentheses around the '&' expression to silence this warning}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
-
-  (void)(i ^ i & i); // expected-warning {{'&' within '^'}} \
-                     // expected-note {{place parentheses around the '&' expression to silence this warning}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:14-[[@LINE-2]]:14}:"("
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:19-[[@LINE-3]]:19}:")"
-
-  (void)(i ||
-             i && i); // expected-warning {{'&&' within '||'}} \
-                      // expected-note {{place parentheses around the '&&' expression to silence this warning}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:14-[[@LINE-2]]:14}:"("
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:20}:")"
-
-  (void)(i || i && "w00t"); // no warning.
-  (void)("w00t" && i || i); // no warning.
-
-  (void)(i || i && "w00t" || i); // expected-warning {{'&&' within '||'}} \
-                                 // expected-note {{place parentheses around the '&&' expression to silence this warning}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"("
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:")"
-
-  (void)(i || "w00t" && i || i); // expected-warning {{'&&' within '||'}} \
-                                 // expected-note {{place parentheses around the '&&' expression to silence this warning}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"("
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:")"
-
-  (void)(i && i || 0); // no warning.
-  (void)(0 || i && i); // no warning.
 }
 
 _Bool someConditionFunc();
 
 void conditional_op(int x, int y, _Bool b, void* p) {
   (void)(x + someConditionFunc() ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '+'}} \
                                            // expected-note {{place parentheses around the '+' expression to silence this warning}} \
                                            // expected-note {{place parentheses around the '?:' expression to evaluate it first}}

cfe/trunk/test/SemaCXX/parentheses.cpp

-// RUN: %clang_cc1 -verify %s
+// RUN: %clang_cc1 -verify -Wlogical-op-parentheses %s
 
 // PR16930, PR16727:
 template<class Foo>
 bool test(Foo f, int *array)
 {
   return false && false || array[f.get()]; // expected-warning {{'&&' within '||'}} expected-note {{parentheses}}
 }