[PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators

[David Blaikie via cfe-commits]

Do you have some data on the true/false positive rate for this warning?

On Fri, Sep 23, 2016 at 6:12 AM Daniel Marjamäki <
daniel.marjamaki at evidente.se> wrote:

> danielmarjamaki created this revision.
> danielmarjamaki added reviewers: dblaikie, rtrieu.
> danielmarjamaki added a subscriber: cfe-commits.
> danielmarjamaki set the repository for this revision to rL LLVM.
>
> 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.
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D24861
>
> Files:
>   lib/Sema/SemaExpr.cpp
>   test/Sema/parentheses.cpp
>
> Index: test/Sema/parentheses.cpp
> ===================================================================
> --- test/Sema/parentheses.cpp
> +++ test/Sema/parentheses.cpp
> @@ -95,6 +95,21 @@
>    // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
>    // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
>
> +  (void)(a >> b * c); // expected-warning {{operator '>>' has lower
> precedence than '*'; '*' will be evaluated first}} \
> +                         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]]:20-[[@LINE-3]]:20}:")"
> +
> +  (void)(a / b << c); // expected-warning {{operator '<<' has lower
> precedence than '/'; '/' will be evaluated first}} \
> +                         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)(a % b << c); // expected-warning {{operator '<<' has lower
> precedence than '%'; '%' will be evaluated first}} \
> +                         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}:")"
> +
>    Stream() << b + c;
>    Stream() >> b + c; // expected-warning {{operator '>>' has lower
> precedence than '+'; '+' will be evaluated first}} \
>                          expected-note {{place parentheses around the '+'
> expression to silence this warning}}
> Index: lib/Sema/SemaExpr.cpp
> ===================================================================
> --- lib/Sema/SemaExpr.cpp
> +++ lib/Sema/SemaExpr.cpp
> @@ -11246,10 +11246,10 @@
>    }
>  }
>
> -static void DiagnoseAdditionInShift(Sema &S, SourceLocation OpLoc,
> +static void DiagnoseAddOrMulInShift(Sema &S, SourceLocation OpLoc,
>                                      Expr *SubExpr, StringRef Shift) {
>    if (BinaryOperator *Bop = dyn_cast<BinaryOperator>(SubExpr)) {
> -    if (Bop->getOpcode() == BO_Add || Bop->getOpcode() == BO_Sub) {
> +    if (Bop->isAdditiveOp() || Bop->isMultiplicativeOp()) {
>        StringRef Op = Bop->getOpcodeStr();
>        S.Diag(Bop->getOperatorLoc(), diag::warn_addition_in_bitshift)
>            << Bop->getSourceRange() << OpLoc << Shift << Op;
> @@ -11313,8 +11313,8 @@
>    if ((Opc == BO_Shl &&
> LHSExpr->getType()->isIntegralType(Self.getASTContext()))
>        || Opc == BO_Shr) {
>      StringRef Shift = BinaryOperator::getOpcodeStr(Opc);
> -    DiagnoseAdditionInShift(Self, OpLoc, LHSExpr, Shift);
> -    DiagnoseAdditionInShift(Self, OpLoc, RHSExpr, Shift);
> +    DiagnoseAddOrMulInShift(Self, OpLoc, LHSExpr, Shift);
> +    DiagnoseAddOrMulInShift(Self, OpLoc, RHSExpr, Shift);
>    }
>
>    // Warn on overloaded shift operators and comparisons, such as:
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160923/192119df/attachment.html>