[PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators
danielmarjamaki abandoned this revision. danielmarjamaki added a comment. I will not work on this in the near future Repository: rL LLVM https://reviews.llvm.org/D24861 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators
danielmarjamaki added a comment. Ping Repository: rL LLVM https://reviews.llvm.org/D24861 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators
danielmarjamaki set the repository for this revision to rL LLVM. danielmarjamaki updated this revision to Diff 73633. danielmarjamaki added a comment. Add new flag : -Wshift-op-parentheses-mul This is not enabled by default. Repository: rL LLVM https://reviews.llvm.org/D24861 Files: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaExpr.cpp test/Sema/parentheses.cpp Index: test/Sema/parentheses.cpp === --- test/Sema/parentheses.cpp +++ test/Sema/parentheses.cpp @@ -1,5 +1,5 @@ -// RUN: %clang_cc1 -Wparentheses -fsyntax-only -verify %s -// RUN: %clang_cc1 -Wparentheses -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s +// RUN: %clang_cc1 -Wparentheses -Wshift-op-parentheses-mul -fsyntax-only -verify %s +// RUN: %clang_cc1 -Wparentheses -Wshift-op-parentheses-mul -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s bool someConditionFunc(); @@ -95,6 +95,23 @@ // 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}:")" + + (void)(a * b << c); // no warning, often the execution order does not matter. + 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 @@ -11263,18 +11263,26 @@ } } -static void DiagnoseAdditionInShift(Sema &S, SourceLocation OpLoc, -Expr *SubExpr, StringRef Shift) { - if (BinaryOperator *Bop = dyn_cast(SubExpr)) { -if (Bop->getOpcode() == BO_Add || Bop->getOpcode() == BO_Sub) { - StringRef Op = Bop->getOpcodeStr(); - S.Diag(Bop->getOperatorLoc(), diag::warn_addition_in_bitshift) - << Bop->getSourceRange() << OpLoc << Shift << Op; - SuggestParentheses(S, Bop->getOperatorLoc(), - S.PDiag(diag::note_precedence_silence) << Op, - Bop->getSourceRange()); -} - } +static void DiagnoseAddOrMulInShift(Sema &S, SourceLocation OpLoc, +Expr *SubExpr, StringRef Shift, +bool ShiftLeftLHS) { + BinaryOperator *Bop = dyn_cast(SubExpr); + if (!Bop) +return; + if (!Bop->isAdditiveOp() && !Bop->isMultiplicativeOp()) +return; + // In many cases, execution order does not matter for 'A*BgetOpcodeStr(); + S.Diag(Bop->getOperatorLoc(), Bop->isAdditiveOp() +? diag::warn_addition_in_bitshift +: diag::warn_multiplication_in_bitshift) + << Bop->getSourceRange() << OpLoc << Shift << Op; + SuggestParentheses(S, Bop->getOperatorLoc(), + S.PDiag(diag::note_precedence_silence) << Op, + Bop->getSourceRange()); } static void DiagnoseShiftCompare(Sema &S, SourceLocation OpLoc, @@ -11330,8 +11338,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, Opc == BO_Shl); +DiagnoseAddOrMulInShift(Self, OpLoc, RHSExpr, Shift, false); } // Warn on overloaded shift operators and comparisons, such as: Index: include/clang/Basic/DiagnosticSemaKinds.td === --- include/clang/Basic/DiagnosticSemaKinds.td +++ include
Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators
This warning group (-Wshift-op-parentheses) is already a subgroup of -Wparentheses. The suggestion of moving to a sub group was to create a new subgroup, either as a child of -Wshift-op-parentheses or -Wparentheses, so that this new warning can be turned on and off independently and allow it to be off by default. On Mon, Oct 3, 2016 at 3:26 AM, Daniel Marjamäki < daniel.marjam...@evidente.se> wrote: > Hello! > > Moving it to a subgroup such as -Wparentheses is fine for me. I will look > at that when I have time. Do you think this warning has an acceptable > output then? > > Best regards, > Daniel Marjamäki > > > .. > Daniel Marjamäki Senior Engineer > Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden > > Mobile: +46 (0)709 12 42 62 > E-mail: daniel.marjam...@evidente.se > > www.evidente.se > > > Från: Richard Trieu [rtr...@google.com] > Skickat: den 1 oktober 2016 00:41 > Till: reviews+d24861+public+1ab88c6ac93f5...@reviews.llvm.org > Kopia: Daniel Marjamäki; David Blaikie; jo...@netbsd.org; > bruno.card...@gmail.com; cfe-commits > Ämne: Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns > for multiplicative operators > > Currently, this warning is on by default. As you said, the results you > found look intentional in many cases, so there is a high false positive > rate. For on by default warnings, we expect a high true positive rate and > intend for users to not disable the warning. From my analysis on a > separate codebase, I found less than 10% true positive rate out of 200 > warnings. One option might be to move this warning to a subgroup, which > would leave it discoverable from either -Wall or -Wparentheses, but not > have it on by default. > > On Wed, Sep 28, 2016 at 4:09 AM, Daniel Marjamäki < > daniel.marjam...@evidente.se<mailto:daniel.marjam...@evidente.se>> wrote: > danielmarjamaki added a comment. > > 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. > > > https://reviews.llvm.org/D24861 > > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators
On Sat, Oct 1, 2016 at 9:34 AM, Joerg Sonnenberger via cfe-commits < cfe-commits@lists.llvm.org> wrote: > On Fri, Sep 30, 2016 at 03:41:53PM -0700, Richard Trieu via cfe-commits > wrote: > > Currently, this warning is on by default. As you said, the results you > > found look intentional in many cases, so there is a high false positive > > rate. For on by default warnings, we expect a high true positive rate > and > > intend for users to not disable the warning. From my analysis on a > > separate codebase, I found less than 10% true positive rate out of 200 > > warnings. One option might be to move this warning to a subgroup, which > > would leave it discoverable from either -Wall or -Wparentheses, but not > > have it on by default. > > We are now only talking about the right shift version, are we? That > seems to me to be much less intrusive, but should belong into the same > subgroup as the add checks. > > Joerg > I used the updated patch for the warning, which has both left and right shifts, with an exception for "A * B << C". The number of right shift versus left shift warnings is similar with similar true positive rate. ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
SV: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators
Hello! Moving it to a subgroup such as -Wparentheses is fine for me. I will look at that when I have time. Do you think this warning has an acceptable output then? Best regards, Daniel Marjamäki .. Daniel Marjamäki Senior Engineer Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden Mobile: +46 (0)709 12 42 62 E-mail: daniel.marjam...@evidente.se www.evidente.se Från: Richard Trieu [rtr...@google.com] Skickat: den 1 oktober 2016 00:41 Till: reviews+d24861+public+1ab88c6ac93f5...@reviews.llvm.org Kopia: Daniel Marjamäki; David Blaikie; jo...@netbsd.org; bruno.card...@gmail.com; cfe-commits Ämne: Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators Currently, this warning is on by default. As you said, the results you found look intentional in many cases, so there is a high false positive rate. For on by default warnings, we expect a high true positive rate and intend for users to not disable the warning. From my analysis on a separate codebase, I found less than 10% true positive rate out of 200 warnings. One option might be to move this warning to a subgroup, which would leave it discoverable from either -Wall or -Wparentheses, but not have it on by default. On Wed, Sep 28, 2016 at 4:09 AM, Daniel Marjamäki mailto:daniel.marjam...@evidente.se>> wrote: danielmarjamaki added a comment. 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. https://reviews.llvm.org/D24861 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators
On Fri, Sep 30, 2016 at 03:41:53PM -0700, Richard Trieu via cfe-commits wrote: > Currently, this warning is on by default. As you said, the results you > found look intentional in many cases, so there is a high false positive > rate. For on by default warnings, we expect a high true positive rate and > intend for users to not disable the warning. From my analysis on a > separate codebase, I found less than 10% true positive rate out of 200 > warnings. One option might be to move this warning to a subgroup, which > would leave it discoverable from either -Wall or -Wparentheses, but not > have it on by default. We are now only talking about the right shift version, are we? That seems to me to be much less intrusive, but should belong into the same subgroup as the add checks. Joerg ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators
Currently, this warning is on by default. As you said, the results you found look intentional in many cases, so there is a high false positive rate. For on by default warnings, we expect a high true positive rate and intend for users to not disable the warning. From my analysis on a separate codebase, I found less than 10% true positive rate out of 200 warnings. One option might be to move this warning to a subgroup, which would leave it discoverable from either -Wall or -Wparentheses, but not have it on by default. On Wed, Sep 28, 2016 at 4:09 AM, Daniel Marjamäki < daniel.marjam...@evidente.se> wrote: > danielmarjamaki added a comment. > > 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. > > > https://reviews.llvm.org/D24861 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators
danielmarjamaki added a comment. 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. https://reviews.llvm.org/D24861 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators
danielmarjamaki removed rL LLVM as the repository for this revision. danielmarjamaki updated this revision to Diff 72797. danielmarjamaki added a comment. Don't write warning for multiplication in LHS of <<. Often the execution order is not important. 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,23 @@ // 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}:")" + + (void)(a * b << c); // no warning, often the execution order does not matter. + 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,18 +11246,24 @@ } } -static void DiagnoseAdditionInShift(Sema &S, SourceLocation OpLoc, -Expr *SubExpr, StringRef Shift) { - if (BinaryOperator *Bop = dyn_cast(SubExpr)) { -if (Bop->getOpcode() == BO_Add || Bop->getOpcode() == BO_Sub) { - StringRef Op = Bop->getOpcodeStr(); - S.Diag(Bop->getOperatorLoc(), diag::warn_addition_in_bitshift) - << Bop->getSourceRange() << OpLoc << Shift << Op; - SuggestParentheses(S, Bop->getOperatorLoc(), - S.PDiag(diag::note_precedence_silence) << Op, - Bop->getSourceRange()); -} - } +static void DiagnoseAddOrMulInShift(Sema &S, SourceLocation OpLoc, +Expr *SubExpr, StringRef Shift, +bool ShiftLeftLHS) { + BinaryOperator *Bop = dyn_cast(SubExpr); + if (!Bop) +return; + if (!Bop->isAdditiveOp() && !Bop->isMultiplicativeOp()) +return; + // In many cases, execution order does not matter for 'A*BgetOpcodeStr(); + S.Diag(Bop->getOperatorLoc(), diag::warn_addition_in_bitshift) + << Bop->getSourceRange() << OpLoc << Shift << Op; + SuggestParentheses(S, Bop->getOperatorLoc(), + S.PDiag(diag::note_precedence_silence) << Op, + Bop->getSourceRange()); } static void DiagnoseShiftCompare(Sema &S, SourceLocation OpLoc, @@ -11313,8 +11319,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, Opc == BO_Shl); +DiagnoseAddOrMulInShift(Self, OpLoc, RHSExpr, Shift, false); } // Warn on overloaded shift operators and comparisons, such as: Index: test/Sema/parentheses.cpp === --- test/Sema/parentheses.cpp +++ test/Sema/parentheses.cpp @@ -95,6 +95,23 @@ // 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 '
Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators
joerg added a subscriber: joerg. joerg added a comment. 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. Repository: rL LLVM https://reviews.llvm.org/D24861 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators
I'd still wonder if this meets the bar for false positives (generally we try to only add warnings that would be enabled by default, even in new codebases - where most of what they find in a newcodebase are latent bugs, not noise (so the cleanup is at least fairly justified as being beneficial in itself rather than only as a means to enable the warning to catch some bugs in the future)) But I know we made some exception for the &&/|| version of -Wparentheses, so maybe this variation meets that same bar. (if Richard Trieu doesn't have enough context to make that call, I'd probably rope in Richard Smith (& possibly Chandler Carruth - I recall him commenting on the &&/|| -Wparentheses form on more than one occasion)) On Tue, Sep 27, 2016 at 10:01 AM Bruno Cardoso Lopes < bruno.card...@gmail.com> wrote: > bruno added a subscriber: bruno. > bruno added a comment. > > Hi Daniel, > > This is very nice. > > In https://reviews.llvm.org/D24861#553606, @danielmarjamaki wrote: > > > 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< 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? > > > Repository: > rL LLVM > > https://reviews.llvm.org/D24861 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators
bruno added a subscriber: bruno. bruno added a comment. Hi Daniel, This is very nice. In https://reviews.llvm.org/D24861#553606, @danielmarjamaki wrote: > 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< 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? Repository: rL LLVM https://reviews.llvm.org/D24861 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators
danielmarjamaki added a comment. 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
Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators
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.marjam...@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(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: > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators
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(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: 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