From 82214a80c0163e01e4d8dec1426023c89277dbb4 Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Fri, 18 Feb 2011 23:54:50 +0000 Subject: [PATCH] Initial steps to improve diagnostics when there is a NULL and a non-pointer on the two sides of a conditional expression. Patch by Stephen Hines and Mihai Rusu. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@125995 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/AST/Expr.h | 27 ++++++++++--- include/clang/Basic/DiagnosticSemaKinds.td | 2 + include/clang/Sema/Sema.h | 3 ++ lib/AST/Expr.cpp | 30 ++++++++------ lib/Sema/SemaExpr.cpp | 46 ++++++++++++++++++++++ lib/Sema/SemaExprCXX.cpp | 20 +++++++--- test/Sema/conditional-expr.c | 13 ++++++ test/SemaCXX/__null.cpp | 7 ++++ test/SemaCXX/conditional-expr.cpp | 12 ++++++ test/SemaCXX/nullptr.cpp | 2 + 10 files changed, 140 insertions(+), 22 deletions(-) diff --git a/include/clang/AST/Expr.h b/include/clang/AST/Expr.h index 36644784c9..a17205c2b6 100644 --- a/include/clang/AST/Expr.h +++ b/include/clang/AST/Expr.h @@ -437,6 +437,22 @@ public: /// EvaluateAsLValue - Evaluate an expression to see if it's a lvalue. bool EvaluateAsAnyLValue(EvalResult &Result, const ASTContext &Ctx) const; + /// \brief Enumeration used to describe the kind of Null pointer constant + /// returned from \c isNullPointerConstant(). + enum NullPointerConstantKind { + /// \brief Expression is not a Null pointer constant. + NPCK_NotNull = 0, + + /// \brief Expression is a Null pointer constant built from a zero integer. + NPCK_ZeroInteger, + + /// \brief Expression is a C++0X nullptr. + NPCK_CXX0X_nullptr, + + /// \brief Expression is a GNU-style __null constant. + NPCK_GNUNull + }; + /// \brief Enumeration used to describe how \c isNullPointerConstant() /// should cope with value-dependent expressions. enum NullPointerConstantValueDependence { @@ -452,11 +468,12 @@ public: NPC_ValueDependentIsNotNull }; - /// isNullPointerConstant - C99 6.3.2.3p3 - Return true if this is either an - /// integer constant expression with the value zero, or if this is one that is - /// cast to void*. - bool isNullPointerConstant(ASTContext &Ctx, - NullPointerConstantValueDependence NPC) const; + /// isNullPointerConstant - C99 6.3.2.3p3 - Test if this reduces down to + /// a Null pointer constant. The return value can further distinguish the + /// kind of NULL pointer constant that was detected. + NullPointerConstantKind isNullPointerConstant( + ASTContext &Ctx, + NullPointerConstantValueDependence NPC) const; /// isOBJCGCCandidate - Return true if this expression may be used in a read/ /// write barrier. diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 7b28463349..9592cbb36f 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -3147,6 +3147,8 @@ def err_incomplete_type_used_in_type_trait_expr : Error< "incomplete type %0 used in type trait expression">; def err_expected_ident_or_lparen : Error<"expected identifier or '('">; +def err_typecheck_cond_incompatible_operands_null : Error< + "non-pointer operand type %0 incompatible with %select{NULL|nullptr}1">; } // End of general sema category. // inline asm. diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index 8d8cf09912..4e3173ab09 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -4774,6 +4774,9 @@ public: QualType FindCompositeObjCPointerType(Expr *&LHS, Expr *&RHS, SourceLocation questionLoc); + bool DiagnoseConditionalForNull(Expr *LHS, Expr *RHS, + SourceLocation QuestionLoc); + /// type checking for vector binary operators. QualType CheckVectorOperands(SourceLocation l, Expr *&lex, Expr *&rex); QualType CheckVectorCompareOperands(Expr *&lex, Expr *&rx, diff --git a/lib/AST/Expr.cpp b/lib/AST/Expr.cpp index 7e46d4fe45..391b26ab48 100644 --- a/lib/AST/Expr.cpp +++ b/lib/AST/Expr.cpp @@ -2132,11 +2132,14 @@ bool Expr::isConstantInitializer(ASTContext &Ctx, bool IsForRef) const { return isEvaluatable(Ctx); } -/// isNullPointerConstant - C99 6.3.2.3p3 - Return true if this is either an -/// integer constant expression with the value zero, or if this is one that is -/// cast to void*. -bool Expr::isNullPointerConstant(ASTContext &Ctx, - NullPointerConstantValueDependence NPC) const { +/// isNullPointerConstant - C99 6.3.2.3p3 - Return whether this is a null +/// pointer constant or not, as well as the specific kind of constant detected. +/// Null pointer constants can be integer constant expressions with the +/// value zero, casts of zero to void*, nullptr (C++0X), or __null +/// (a GNU extension). +Expr::NullPointerConstantKind +Expr::isNullPointerConstant(ASTContext &Ctx, + NullPointerConstantValueDependence NPC) const { if (isValueDependent()) { switch (NPC) { case NPC_NeverValueDependent: @@ -2144,10 +2147,13 @@ bool Expr::isNullPointerConstant(ASTContext &Ctx, // If the unthinkable happens, fall through to the safest alternative. case NPC_ValueDependentIsNull: - return isTypeDependent() || getType()->isIntegralType(Ctx); + if (isTypeDependent() || getType()->isIntegralType(Ctx)) + return NPCK_ZeroInteger; + else + return NPCK_NotNull; case NPC_ValueDependentIsNotNull: - return false; + return NPCK_NotNull; } } @@ -2176,12 +2182,12 @@ bool Expr::isNullPointerConstant(ASTContext &Ctx, return DefaultArg->getExpr()->isNullPointerConstant(Ctx, NPC); } else if (isa(this)) { // The GNU __null extension is always a null pointer constant. - return true; + return NPCK_GNUNull; } // C++0x nullptr_t is always a null pointer constant. if (getType()->isNullPtrType()) - return true; + return NPCK_CXX0X_nullptr; if (const RecordType *UT = getType()->getAsUnionType()) if (UT && UT->getDecl()->hasAttr()) @@ -2193,12 +2199,14 @@ bool Expr::isNullPointerConstant(ASTContext &Ctx, // This expression must be an integer type. if (!getType()->isIntegerType() || (Ctx.getLangOptions().CPlusPlus && getType()->isEnumeralType())) - return false; + return NPCK_NotNull; // If we have an integer constant expression, we need to *evaluate* it and // test for the value 0. llvm::APSInt Result; - return isIntegerConstantExpr(Result, Ctx) && Result == 0; + bool IsNull = isIntegerConstantExpr(Result, Ctx) && Result == 0; + + return (IsNull ? NPCK_ZeroInteger : NPCK_NotNull); } /// \brief If this expression is an l-value for an Objective C diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index 7e6eee7fb7..792eb8af98 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/lib/Sema/SemaExpr.cpp @@ -5229,6 +5229,46 @@ ExprResult Sema::ActOnParenOrParenListExpr(SourceLocation L, return Owned(expr); } +/// \brief Emit a specialized diagnostic when one expression is a null pointer +/// constant and the other is not a pointer. +bool Sema::DiagnoseConditionalForNull(Expr *LHS, Expr *RHS, + SourceLocation QuestionLoc) { + Expr *NullExpr = LHS; + Expr *NonPointerExpr = RHS; + Expr::NullPointerConstantKind NullKind = + NullExpr->isNullPointerConstant(Context, + Expr::NPC_ValueDependentIsNotNull); + + if (NullKind == Expr::NPCK_NotNull) { + NullExpr = RHS; + NonPointerExpr = LHS; + NullKind = + NullExpr->isNullPointerConstant(Context, + Expr::NPC_ValueDependentIsNotNull); + } + + if (NullKind == Expr::NPCK_NotNull) + return false; + + if (NullKind == Expr::NPCK_ZeroInteger) { + // In this case, check to make sure that we got here from a "NULL" + // string in the source code. + NullExpr = NullExpr->IgnoreParenImpCasts(); + SourceManager& SM = Context.getSourceManager(); + SourceLocation Loc = SM.getInstantiationLoc(NullExpr->getExprLoc()); + unsigned Len = + Lexer::MeasureTokenLength(Loc, SM, Context.getLangOptions()); + if (Len != 4 || memcmp(SM.getCharacterData(Loc), "NULL", 4)) + return false; + } + + int DiagType = (NullKind == Expr::NPCK_CXX0X_nullptr); + Diag(QuestionLoc, diag::err_typecheck_cond_incompatible_operands_null) + << NonPointerExpr->getType() << DiagType + << NonPointerExpr->getSourceRange(); + return true; +} + /// Note that lhs is not null here, even if this is the gnu "x ?: y" extension. /// In that case, lhs = cond. /// C99 6.5.15 @@ -5471,6 +5511,12 @@ QualType Sema::CheckConditionalOperands(Expr *&Cond, Expr *&LHS, Expr *&RHS, return LHSTy; } + // Emit a better diagnostic if one of the expressions is a null pointer + // constant and the other is not a pointer type. In this case, the user most + // likely forgot to take the address of the other expression. + if (DiagnoseConditionalForNull(LHS, RHS, QuestionLoc)) + return QualType(); + // Otherwise, the operands are not compatible. Diag(QuestionLoc, diag::err_typecheck_cond_incompatible_operands) << LHSTy << RHSTy << LHS->getSourceRange() << RHS->getSourceRange(); diff --git a/lib/Sema/SemaExprCXX.cpp b/lib/Sema/SemaExprCXX.cpp index 44508540d0..b78e52303e 100644 --- a/lib/Sema/SemaExprCXX.cpp +++ b/lib/Sema/SemaExprCXX.cpp @@ -2874,13 +2874,14 @@ static bool TryClassUnification(Sema &Self, Expr *From, Expr *To, /// value operand is a class type, overload resolution is used to find a /// conversion to a common type. static bool FindConditionalOverload(Sema &Self, Expr *&LHS, Expr *&RHS, - SourceLocation Loc) { + SourceLocation QuestionLoc) { Expr *Args[2] = { LHS, RHS }; - OverloadCandidateSet CandidateSet(Loc); - Self.AddBuiltinOperatorCandidates(OO_Conditional, Loc, Args, 2, CandidateSet); + OverloadCandidateSet CandidateSet(QuestionLoc); + Self.AddBuiltinOperatorCandidates(OO_Conditional, QuestionLoc, Args, 2, + CandidateSet); OverloadCandidateSet::iterator Best; - switch (CandidateSet.BestViableFunction(Self, Loc, Best)) { + switch (CandidateSet.BestViableFunction(Self, QuestionLoc, Best)) { case OR_Success: // We found a match. Perform the conversions on the arguments and move on. if (Self.PerformImplicitConversion(LHS, Best->BuiltinTypes.ParamTypes[0], @@ -2891,13 +2892,20 @@ static bool FindConditionalOverload(Sema &Self, Expr *&LHS, Expr *&RHS, return false; case OR_No_Viable_Function: - Self.Diag(Loc, diag::err_typecheck_cond_incompatible_operands) + + // Emit a better diagnostic if one of the expressions is a null pointer + // constant and the other is a pointer type. In this case, the user most + // likely forgot to take the address of the other expression. + if (Self.DiagnoseConditionalForNull(LHS, RHS, QuestionLoc)) + return true; + + Self.Diag(QuestionLoc, diag::err_typecheck_cond_incompatible_operands) << LHS->getType() << RHS->getType() << LHS->getSourceRange() << RHS->getSourceRange(); return true; case OR_Ambiguous: - Self.Diag(Loc, diag::err_conditional_ambiguous_ovl) + Self.Diag(QuestionLoc, diag::err_conditional_ambiguous_ovl) << LHS->getType() << RHS->getType() << LHS->getSourceRange() << RHS->getSourceRange(); // FIXME: Print the possible common types by printing the return types of diff --git a/test/Sema/conditional-expr.c b/test/Sema/conditional-expr.c index 6e248bc3cf..7a8c9e9f36 100644 --- a/test/Sema/conditional-expr.c +++ b/test/Sema/conditional-expr.c @@ -75,3 +75,16 @@ int f2(int x) { // We can suppress this because the immediate context wants an int. return (x != 0) ? 0U : x; } + +#define NULL (void*)0 + +void PR9236() { + struct A {int i;} A1; + (void)(1 ? A1 : NULL); // expected-error{{non-pointer operand type 'struct A' incompatible with NULL}} + (void)(1 ? NULL : A1); // expected-error{{non-pointer operand type 'struct A' incompatible with NULL}} + (void)(1 ? 0 : A1); // expected-error{{incompatible operand types}} + (void)(1 ? (void*)0 : A1); // expected-error{{incompatible operand types}} + (void)(1 ? A1: (void*)0); // expected-error{{incompatible operand types}} + (void)(1 ? A1 : (NULL)); // expected-error{{non-pointer operand type 'struct A' incompatible with NULL}} +} + diff --git a/test/SemaCXX/__null.cpp b/test/SemaCXX/__null.cpp index 3583655134..1989a45fb5 100644 --- a/test/SemaCXX/__null.cpp +++ b/test/SemaCXX/__null.cpp @@ -12,3 +12,10 @@ void f() { // Verify that null is evaluated as 0. int b[__null ? -1 : 1]; } + +struct A {}; + +void g() { + (void)(0 ? __null : A()); // expected-error {{non-pointer operand type 'A' incompatible with NULL}} + (void)(0 ? A(): __null); // expected-error {{non-pointer operand type 'A' incompatible with NULL}} +} diff --git a/test/SemaCXX/conditional-expr.cpp b/test/SemaCXX/conditional-expr.cpp index 8ac0a9736a..3881765cff 100644 --- a/test/SemaCXX/conditional-expr.cpp +++ b/test/SemaCXX/conditional-expr.cpp @@ -307,3 +307,15 @@ namespace PR7598 { } } + +namespace PR9236 { +#define NULL 0L + void f() { + (void)(true ? A() : NULL); // expected-error{{non-pointer operand type 'A' incompatible with NULL}} + (void)(true ? NULL : A()); // expected-error{{non-pointer operand type 'A' incompatible with NULL}} + (void)(true ? 0 : A()); // expected-error{{incompatible operand types}} + (void)(true ? nullptr : A()); // expected-error{{non-pointer operand type 'A' incompatible with nullptr}} + (void)(true ? __null : A()); // expected-error{{non-pointer operand type 'A' incompatible with NULL}} + (void)(true ? (void*)0 : A()); // expected-error{{incompatible operand types}} + } +} diff --git a/test/SemaCXX/nullptr.cpp b/test/SemaCXX/nullptr.cpp index 666701c3c6..7385fd42ad 100644 --- a/test/SemaCXX/nullptr.cpp +++ b/test/SemaCXX/nullptr.cpp @@ -46,6 +46,8 @@ nullptr_t f(nullptr_t null) (void)(1 + nullptr); // expected-error {{invalid operands to binary expression}} (void)(0 ? nullptr : 0); // expected-error {{incompatible operand types}} (void)(0 ? nullptr : (void*)0); + (void)(0 ? nullptr : A()); // expected-error {{non-pointer operand type 'A' incompatible with nullptr}} + (void)(0 ? A() : nullptr); // expected-error {{non-pointer operand type 'A' incompatible with nullptr}} // Overloading int t = o1(nullptr);