This is an archive of the discontinued LLVM Phabricator instance.

[Sema] extend Wshift-op-parentheses so it warns for multiplicative operators
AbandonedPublic

Authored by danielmarjamaki on Sep 23 2016, 6:12 AM.

Details

Reviewers
rtrieu
Summary

This patch makes Clang warn about following code:

a = (b * c >> 2);

It might be a good idea to use parentheses to clarify the operator precedence.

Diff Detail

Event Timeline

danielmarjamaki retitled this revision from to [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators.
danielmarjamaki updated this object.
danielmarjamaki added reviewers: dblaikie, rtrieu.
danielmarjamaki set the repository for this revision to rL LLVM.
danielmarjamaki added a subscriber: cfe-commits.

Compiling 2064 projects resulted in 904 warnings

Here are the results:
https://drive.google.com/file/d/0BykPmWrCOxt2N04tYl8zVHA3MXc/view?usp=sharing

The results looks acceptable imho. The code looks intentional in many cases so I believe there are users that will disable this warning. Probably there are true positives where the evaluation order is not really known. There were many warnings about macro arguments where the macro bitshifts the argument - these macros look very shaky to me.

I saw some warnings about such code:

a * b << c

Maybe we should not warn about this. As far as I see, the result will be the same if (a*b) or (b<<c) is evaluated first - unless there is some overflow or signedness issues. What do you think? I'll keep these warnings for now.

bruno added a subscriber: bruno.Sep 27 2016, 10:01 AM

Hi Daniel,

This is very nice.

Compiling 2064 projects resulted in 904 warnings

Here are the results:
https://drive.google.com/file/d/0BykPmWrCOxt2N04tYl8zVHA3MXc/view?usp=sharing

The results looks acceptable imho. The code looks intentional in many cases so I believe there are users that will disable this warning. Probably there are true positives where the evaluation order is not really known. There were many warnings about macro arguments where the macro bitshifts the argument - these macros look very shaky to me.

I saw some warnings about such code:

a * b << c

Maybe we should not warn about this. As far as I see, the result will be the same if (a*b) or (b<<c) is evaluated first - unless there is some overflow or signedness issues. What do you think? I'll keep these warnings for now.

Any idea on how expensive would be to reason about these false positives and avoid them?

joerg added a subscriber: joerg.Sep 27 2016, 2:52 PM

I think the comment from Daniel shows the crux of the issue. A left shift is by nature a multiplication operation, so I don't see why it should get the warning. A right shift works like a division and order is quite significant for that.

danielmarjamaki removed rL LLVM as the repository for this revision.

Don't write warning for multiplication in LHS of <<. Often the execution order is not important.

I updated the patch so it does not warn about 'A * B << C'. It's a simple fix. I have not made careful measurements but I guess that the performance penalty is acceptable.

danielmarjamaki set the repository for this revision to rL LLVM.

Add new flag : -Wshift-op-parentheses-mul
This is not enabled by default.

danielmarjamaki abandoned this revision.Feb 23 2017, 11:17 AM

I will not work on this in the near future