From 9cfdae3144fc004cf203b05288f4dab49097ce7b Mon Sep 17 00:00:00 2001 From: Hans Wennborg Date: Fri, 3 Jun 2011 18:00:36 +0000 Subject: [PATCH] Warn about missing parentheses for conditional operator. Warn in cases such as "x + someCondition ? 42 : 0;", where the condition expression looks arithmetic, and has a right-hand side that looks boolean. This (partly) addresses http://llvm.org/bugs/show_bug.cgi?id=9969 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@132565 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/DiagnosticSemaKinds.td | 8 ++ lib/Sema/SemaExpr.cpp | 146 +++++++++++++++------ test/Sema/parentheses.c | 24 +++- test/Sema/parentheses.cpp | 18 +++ 4 files changed, 156 insertions(+), 40 deletions(-) create mode 100644 test/Sema/parentheses.cpp diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 95f455f6e4..b6183cad35 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -2562,6 +2562,14 @@ def note_precedence_bitwise_first : Note< def note_precedence_bitwise_silence : Note< "place parentheses around the %0 expression to silence this warning">; +def warn_precedence_conditional : Warning< + "?: has lower precedence than %0; %0 will be evaluated first">, + InGroup; +def note_precedence_conditional_first : Note< + "place parentheses around the ?: expression to evaluate it first">; +def note_precedence_conditional_silence : Note< + "place parentheses around the %0 expression to silence this warning">; + def warn_logical_instead_of_bitwise : Warning< "use of logical %0 with constant operand; switch to bitwise %1 or " "remove constant">, InGroup>; diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index 9afd5f8ccd..1006b15ff1 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/lib/Sema/SemaExpr.cpp @@ -6167,6 +6167,110 @@ QualType Sema::FindCompositeObjCPointerType(ExprResult &LHS, ExprResult &RHS, return QualType(); } +/// SuggestParentheses - Emit a diagnostic together with a fixit hint that wraps +/// ParenRange in parentheses. +static void SuggestParentheses(Sema &Self, SourceLocation Loc, + const PartialDiagnostic &PD, + const PartialDiagnostic &FirstNote, + SourceRange FirstParenRange, + const PartialDiagnostic &SecondNote, + SourceRange SecondParenRange) { + Self.Diag(Loc, PD); + + if (!FirstNote.getDiagID()) + return; + + SourceLocation EndLoc = Self.PP.getLocForEndOfToken(FirstParenRange.getEnd()); + if (!FirstParenRange.getEnd().isFileID() || EndLoc.isInvalid()) { + // We can't display the parentheses, so just return. + return; + } + + Self.Diag(Loc, FirstNote) + << FixItHint::CreateInsertion(FirstParenRange.getBegin(), "(") + << FixItHint::CreateInsertion(EndLoc, ")"); + + if (!SecondNote.getDiagID()) + return; + + EndLoc = Self.PP.getLocForEndOfToken(SecondParenRange.getEnd()); + if (!SecondParenRange.getEnd().isFileID() || EndLoc.isInvalid()) { + // We can't display the parentheses, so just dig the + // warning/error and return. + Self.Diag(Loc, SecondNote); + return; + } + + Self.Diag(Loc, SecondNote) + << FixItHint::CreateInsertion(SecondParenRange.getBegin(), "(") + << FixItHint::CreateInsertion(EndLoc, ")"); +} + +static bool IsArithmeticOp(BinaryOperatorKind Opc) { + return Opc >= BO_Mul && Opc <= BO_Shr; +} + +static bool IsLogicOp(BinaryOperatorKind Opc) { + return (Opc >= BO_LT && Opc <= BO_NE) || (Opc >= BO_LAnd && Opc <= BO_LOr); +} + +/// DiagnoseConditionalPrecedence - Emit a warning when a conditional operator +/// and binary operator are mixed in a way that suggests the programmer assumed +/// the conditional operator has higher precedence, for example: +/// "int x = a + someBinaryCondition ? 1 : 2". +static void DiagnoseConditionalPrecedence(Sema &Self, + SourceLocation OpLoc, + Expr *cond, + Expr *lhs, + Expr *rhs) { + while (ImplicitCastExpr *C = dyn_cast(cond)) + cond = C->getSubExpr(); + + if (BinaryOperator *OP = dyn_cast(cond)) { + if (!IsArithmeticOp(OP->getOpcode())) + return; + + // Drill down on the RHS of the condition. + Expr *CondRHS = OP->getRHS(); + while (ImplicitCastExpr *C = dyn_cast(CondRHS)) + CondRHS = C->getSubExpr(); + while (ParenExpr *P = dyn_cast(CondRHS)) + CondRHS = P->getSubExpr(); + + bool CondRHSLooksBoolean = false; + if (CondRHS->getType()->isBooleanType()) + CondRHSLooksBoolean = true; + else if (BinaryOperator *CondRHSOP = dyn_cast(CondRHS)) + CondRHSLooksBoolean = IsLogicOp(CondRHSOP->getOpcode()); + else if (UnaryOperator *CondRHSOP = dyn_cast(CondRHS)) + CondRHSLooksBoolean = CondRHSOP->getOpcode() == UO_LNot; + + if (CondRHSLooksBoolean) { + // The condition is an arithmetic binary expression, with a right- + // hand side that looks boolean, so warn. + + PartialDiagnostic Warn = Self.PDiag(diag::warn_precedence_conditional) + << OP->getSourceRange() + << BinaryOperator::getOpcodeStr(OP->getOpcode()); + + PartialDiagnostic FirstNote = + Self.PDiag(diag::note_precedence_conditional_silence) + << BinaryOperator::getOpcodeStr(OP->getOpcode()); + + SourceRange FirstParenRange(OP->getLHS()->getLocStart(), + OP->getRHS()->getLocEnd()); + + PartialDiagnostic SecondNote = + Self.PDiag(diag::note_precedence_conditional_first); + SourceRange SecondParenRange(OP->getRHS()->getLocStart(), + rhs->getLocEnd()); + + SuggestParentheses(Self, OpLoc, Warn, FirstNote, FirstParenRange, + SecondNote, SecondParenRange); + } + } +} + /// ActOnConditionalOp - Parse a ?: operation. Note that 'LHS' may be null /// in the case of a the GNU conditional expr extension. ExprResult Sema::ActOnConditionalOp(SourceLocation QuestionLoc, @@ -6211,6 +6315,9 @@ ExprResult Sema::ActOnConditionalOp(SourceLocation QuestionLoc, RHS.isInvalid()) return ExprError(); + DiagnoseConditionalPrecedence(*this, QuestionLoc, Cond.get(), LHS.get(), + RHS.get()); + if (!commonExpr) return Owned(new (Context) ConditionalOperator(Cond.take(), QuestionLoc, LHS.take(), ColonLoc, @@ -8735,45 +8842,6 @@ ExprResult Sema::CreateBuiltinBinOp(SourceLocation OpLoc, CompResultTy, OpLoc)); } -/// SuggestParentheses - Emit a diagnostic together with a fixit hint that wraps -/// ParenRange in parentheses. -static void SuggestParentheses(Sema &Self, SourceLocation Loc, - const PartialDiagnostic &PD, - const PartialDiagnostic &FirstNote, - SourceRange FirstParenRange, - const PartialDiagnostic &SecondNote, - SourceRange SecondParenRange) { - Self.Diag(Loc, PD); - - if (!FirstNote.getDiagID()) - return; - - SourceLocation EndLoc = Self.PP.getLocForEndOfToken(FirstParenRange.getEnd()); - if (!FirstParenRange.getEnd().isFileID() || EndLoc.isInvalid()) { - // We can't display the parentheses, so just return. - return; - } - - Self.Diag(Loc, FirstNote) - << FixItHint::CreateInsertion(FirstParenRange.getBegin(), "(") - << FixItHint::CreateInsertion(EndLoc, ")"); - - if (!SecondNote.getDiagID()) - return; - - EndLoc = Self.PP.getLocForEndOfToken(SecondParenRange.getEnd()); - if (!SecondParenRange.getEnd().isFileID() || EndLoc.isInvalid()) { - // We can't display the parentheses, so just dig the - // warning/error and return. - Self.Diag(Loc, SecondNote); - return; - } - - Self.Diag(Loc, SecondNote) - << FixItHint::CreateInsertion(SecondParenRange.getBegin(), "(") - << FixItHint::CreateInsertion(EndLoc, ")"); -} - /// DiagnoseBitwisePrecedence - Emit a warning when bitwise and comparison /// operators are mixed in a way that suggests that the programmer forgot that /// comparison operators have higher precedence. The most typical example of diff --git a/test/Sema/parentheses.c b/test/Sema/parentheses.c index a25ded66a6..fa3c3c26b0 100644 --- a/test/Sema/parentheses.c +++ b/test/Sema/parentheses.c @@ -39,6 +39,28 @@ void bitwise_rel(unsigned i) { (void)(0 || i && i); // no warning. } +_Bool someConditionFunc(); + +void conditional_op(int x, int y, _Bool b) { + (void)(x + someConditionFunc() ? 1 : 2); // expected-warning {{?: has lower precedence than +}} \ + // expected-note {{place parentheses around the ?: expression to evaluate it first}} \ + // expected-note {{place parentheses around the + expression to silence this warning}} + + (void)(x - b ? 1 : 2); // expected-warning {{?: has lower precedence than -}} \ + // expected-note {{place parentheses around the ?: expression to evaluate it first}} \ + // expected-note {{place parentheses around the - expression to silence this warning}} + + (void)(x * (x == y) ? 1 : 2); // expected-warning {{?: has lower precedence than *}} \ + // expected-note {{place parentheses around the ?: expression to evaluate it first}} \ + // expected-note {{place parentheses around the * expression to silence this warning}} + + (void)(x / !x ? 1 : 2); // expected-warning {{?: has lower precedence than /}} \ + // expected-note {{place parentheses around the ?: expression to evaluate it first}} \ + // expected-note {{place parentheses around the / expression to silence this warning}} + + + (void)(x % 2 ? 1 : 2); // no warning +} + // RUN: %clang_cc1 -fsyntax-only -Wparentheses -Werror -fdiagnostics-show-option %s 2>&1 | FileCheck %s // CHECK: error: using the result of an assignment as a condition without parentheses [-Werror,-Wparentheses] - diff --git a/test/Sema/parentheses.cpp b/test/Sema/parentheses.cpp new file mode 100644 index 0000000000..ad1f399c64 --- /dev/null +++ b/test/Sema/parentheses.cpp @@ -0,0 +1,18 @@ +// RUN: %clang_cc1 -Wparentheses -fsyntax-only -verify %s +// RUN: %clang_cc1 -Wparentheses -fixit %s -o - | %clang_cc1 -Wparentheses -Werror - + +bool someConditionFunc(); + +void conditional_op(int x, int y, bool b) { + (void)(x + someConditionFunc() ? 1 : 2); // expected-warning {{?: has lower precedence than +}} \ + // expected-note {{place parentheses around the ?: expression to evaluate it first}} \ + // expected-note {{place parentheses around the + expression to silence this warning}} + + (void)(x - b ? 1 : 2); // expected-warning {{?: has lower precedence than -}} \ + // expected-note {{place parentheses around the ?: expression to evaluate it first}} \ + // expected-note {{place parentheses around the - expression to silence this warning}} + + (void)(x * (x == y) ? 1 : 2); // expected-warning {{?: has lower precedence than *}} \ + // expected-note {{place parentheses around the ?: expression to evaluate it first}} \ + // expected-note {{place parentheses around the * expression to silence this warning}} +} -- 2.40.0