Page MenuHomePhabricator

Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")
ClosedPublic

Authored by rtrieu on Aug 9 2019, 6:54 PM.

Details

Summary

Extend -Wparentheses to cover mixing bitwise-and and bitwise-or with the conditional operator. There's two main cases seen with this:

unsigned bits1 = 0xf0 | cond ? 0x4 : 0x1;
unsigned bits2 = cond1 ? 0xf0 : 0x10 | cond2 ? 0x5 : 0x2;

// Intended order of evaluation:
unsigned bits1 = 0xf0 | (cond ? 0x4 : 0x1);
unsigned bits2 = (cond1 ? 0xf0 : 0x10) | (cond2 ? 0x5 : 0x2);

// Actual order of evaluation:
unsigned bits1 = (0xf0 | cond) ? 0x4 : 0x1;
unsigned bits2 = cond1 ? 0xf0 : ((0x10 | cond2) ? 0x5 : 0x2);

Diff Detail

Event Timeline

rtrieu created this revision.Aug 9 2019, 6:54 PM
jfb added a subscriber: jfb.Aug 10 2019, 9:01 AM
jfb added inline comments.
test/Sema/parentheses.c
156 ↗(On Diff #214479)

I don't understand why ^ is different. What do you mean by "often used as a logical xor`?

MaskRay added inline comments.
test/Sema/parentheses.c
148 ↗(On Diff #214479)

I hope these | ? : & ? : warnings are disabled-by-default.

rtrieu marked 2 inline comments as done.Aug 12 2019, 2:28 PM
rtrieu added inline comments.
test/Sema/parentheses.c
148 ↗(On Diff #214479)

These new warnings reuse the existing parentheses warnings, which is diag::warn_precedence_conditional. That is on by default, so this one as written is also on by default..

156 ↗(On Diff #214479)

In C++, there's the bitwise operators |, &, and ^. And there's the logical operators || and &&. Since there's no ^^ for a logical-xor, many people will just use the bitwise-xor ^ instead. Since this isn't warning on logical operators, it makes sense to exclude the bitwise-xor that is often used as logical-xor.

jfb added inline comments.Aug 12 2019, 2:46 PM
test/Sema/parentheses.c
156 ↗(On Diff #214479)

So code is often buggy when it uses | and & as diagnosed by this patch, but is rarely buggy when it uses ^?

rtrieu marked an inline comment as done.Aug 12 2019, 3:27 PM
rtrieu added inline comments.
test/Sema/parentheses.c
156 ↗(On Diff #214479)

That's correct. From my testing, &&, || and ^ all had low bug finding rates and didn't make sense to include into this warning while | and & had a high bug finding rate.

jfb added inline comments.Aug 12 2019, 4:11 PM
test/Sema/parentheses.c
156 ↗(On Diff #214479)

OK thanks for clarifying. Could you explain this instead of "logical xor"? It seems useful to know when reading your code that in your experience ^ wasn't a source of bugs as much as the others.

rtrieu updated this revision to Diff 214753.Aug 12 2019, 7:35 PM

Update comments to explain why bitwise-xor is treated as a logical operator.

rtrieu marked an inline comment as done.Aug 12 2019, 7:36 PM
rtrieu added inline comments.
test/Sema/parentheses.c
156 ↗(On Diff #214479)

I have added comments for this test and for the code where xor is excluded.

jfb accepted this revision.Aug 12 2019, 9:39 PM

Thanks!

This revision is now accepted and ready to land.Aug 12 2019, 9:39 PM
MaskRay added inline comments.Aug 12 2019, 9:44 PM
test/Sema/parentheses.c
148 ↗(On Diff #214479)

I agree that

cond1 ? 0xf0 : 0x10 | cond2 ? 0x5 : 0x2; is confusing and justifies a warning. But what is tested here is different.

That is why I created D65192, because such warnings are very annoying as enabled-by-default diagnostics.

I think this change will make it even harder to remove some annoying -Wparentheses warnings..

rtrieu marked an inline comment as done.Aug 13 2019, 4:09 PM
rtrieu added inline comments.
test/Sema/parentheses.c
148 ↗(On Diff #214479)

I can add tests for the other case. This one was used because it was shorter and easier to copy.

Would creating a parentheses subgroup (like -Wbitwise-conditional-parentheses), duping the warning into it, and marking it DefaultIgnore be a better alternative for you?

rtrieu updated this revision to Diff 219213.Sep 6 2019, 6:53 PM

Add more test cases and split new warnings into a separate warning group, but still under -Wparentheses

Do you plan to land it?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFri, Oct 18, 6:52 PM