Page MenuHomePhabricator

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

Authored by MaskRay on Jul 23 2019, 10:43 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Jul 23 2019, 10:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2019, 10:43 PM
MaskRay updated this revision to Diff 211425.Jul 23 2019, 10:45 PM
MaskRay edited the summary of this revision. (Show Details)

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

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.

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 updated this revision to Diff 212265.Jul 29 2019, 7:20 PM
MaskRay edited the summary of this revision. (Show Details)

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

MaskRay marked 3 inline comments as done.Jul 29 2019, 7:29 PM
MaskRay added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
5647–5650 ↗(On Diff #211425)

Restored this one.

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

MaskRay marked an inline comment as done.Jul 31 2019, 7:06 PM

Ping :)

aaron.ballman added inline comments.Aug 1 2019, 6:10 AM
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 marked an inline comment as done.Aug 1 2019, 6:59 AM
MaskRay added inline comments.
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 added inline comments.Aug 1 2019, 9:06 AM
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 marked an inline comment as done.Aug 1 2019, 7:25 PM
MaskRay added inline comments.
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 added a comment.Aug 1 2019, 7:45 PM

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 updated this revision to Diff 212958.Aug 1 2019, 7:58 PM
MaskRay retitled this revision from [Sema] Disable some enabled-by-default -Wparentheses diagnostics to [Sema] Make -Wbitwise-op-parentheses and -Wlogical-op-parentheses disabled-by-default.
MaskRay edited the summary of this revision. (Show Details)

Retitle

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

MaskRay updated this revision to Diff 212962.Aug 1 2019, 8:25 PM

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

aaron.ballman accepted this revision.Aug 2 2019, 5:54 AM

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?

This revision is now accepted and ready to land.Aug 2 2019, 5:54 AM
MaskRay updated this revision to Diff 213064.Aug 2 2019, 9:25 AM

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

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2019, 9:31 AM