Index: include/clang/AST/Decl.h =================================================================== --- include/clang/AST/Decl.h +++ include/clang/AST/Decl.h @@ -850,6 +850,9 @@ return getMostRecentDecl(); } + /// \brief Whether this variable has non-const use so it can't be const. + unsigned NonConstUse:1; + public: typedef redeclarable_base::redecl_range redecl_range; typedef redeclarable_base::redecl_iterator redecl_iterator; @@ -865,6 +868,13 @@ IdentifierInfo *Id, QualType T, TypeSourceInfo *TInfo, StorageClass S); + /// \brief Whether the declared symbol has non-const usage so it can't be + /// const. + bool hasNonConstUse() const { return NonConstUse; } + + /// \brief Use this method when this symbol can't be const. + void setNonConstUse() { NonConstUse = isThisDeclarationReferenced(); } + static VarDecl *CreateDeserialized(ASTContext &C, unsigned ID); SourceRange getSourceRange() const override LLVM_READONLY; Index: include/clang/Basic/DiagnosticGroups.td =================================================================== --- include/clang/Basic/DiagnosticGroups.td +++ include/clang/Basic/DiagnosticGroups.td @@ -433,6 +433,7 @@ def UnusedLabel : DiagGroup<"unused-label">; def UnusedParameter : DiagGroup<"unused-parameter">; def UnusedResult : DiagGroup<"unused-result">; +def NonConstParameter : DiagGroup<"nonconst-parameter">; def PotentiallyEvaluatedExpression : DiagGroup<"potentially-evaluated-expression">; def UnevaluatedExpression : DiagGroup<"unevaluated-expression", [PotentiallyEvaluatedExpression]>; Index: include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- include/clang/Basic/DiagnosticSemaKinds.td +++ include/clang/Basic/DiagnosticSemaKinds.td @@ -197,6 +197,8 @@ def err_parameter_name_omitted : Error<"parameter name omitted">; def warn_unused_parameter : Warning<"unused parameter %0">, InGroup, DefaultIgnore; +def warn_nonconst_parameter : Warning<"parameter %0 can be const">, + InGroup, DefaultIgnore; def warn_unused_variable : Warning<"unused variable %0">, InGroup, DefaultIgnore; def warn_unused_local_typedef : Warning< Index: include/clang/Parse/Parser.h =================================================================== --- include/clang/Parse/Parser.h +++ include/clang/Parse/Parser.h @@ -1395,6 +1395,10 @@ bool IsUnevaluated); private: + /// \brief Mark variable declarations in expression as nonconstuse, to prevent + /// warnings that they can be const. + static void MarkNonConstUse(Expr *E); + ExprResult ParseExpressionWithLeadingAt(SourceLocation AtLoc); ExprResult ParseExpressionWithLeadingExtension(SourceLocation ExtLoc); Index: include/clang/Sema/Sema.h =================================================================== --- include/clang/Sema/Sema.h +++ include/clang/Sema/Sema.h @@ -1762,6 +1762,11 @@ void DiagnoseUnusedParameters(ParmVarDecl * const *Begin, ParmVarDecl * const *End); + /// \brief Diagnose pointer parameters that should be const in the given + /// sequence of ParmVarDecl pointers. + void DiagnoseNonConstPointerParameters(ParmVarDecl * const *Begin, + ParmVarDecl * const *End); + /// \brief Diagnose whether the size of parameters or return value of a /// function or obj-c method definition is pass-by-value and larger than a /// specified threshold. Index: lib/AST/Decl.cpp =================================================================== --- lib/AST/Decl.cpp +++ lib/AST/Decl.cpp @@ -1762,7 +1762,7 @@ IdentifierInfo *Id, QualType T, TypeSourceInfo *TInfo, StorageClass SC) : DeclaratorDecl(DK, DC, IdLoc, Id, T, TInfo, StartLoc), - redeclarable_base(C), Init() { + redeclarable_base(C), Init(), NonConstUse(false) { static_assert(sizeof(VarDeclBitfields) <= sizeof(unsigned), "VarDeclBitfields too large!"); static_assert(sizeof(ParmVarDeclBitfields) <= sizeof(unsigned), Index: lib/Parse/ParseExpr.cpp =================================================================== --- lib/Parse/ParseExpr.cpp +++ lib/Parse/ParseExpr.cpp @@ -167,7 +167,22 @@ ExprResult LHS = ParseCastExpression(/*isUnaryExpression=*/false, /*isAddressOfOperand=*/false, isTypeCast); - return ParseRHSOfBinaryExpression(LHS, prec::Assignment); + + ExprResult ER(ParseRHSOfBinaryExpression(LHS, prec::Assignment)); + if (ER.isInvalid()) + return ER; + + if (auto *B = dyn_cast(ER.get())) { + if (B->isAssignmentOp() || B->isAdditiveOp()) { + MarkNonConstUse(B->getLHS()); + MarkNonConstUse(B->getRHS()); + } + } else if (isa(ER.get()) || + isa(ER.get()) || + isa(ER.get())) { + MarkNonConstUse(ER.get()); + } + return ER; } /// \brief Parse an assignment expression where part of an Objective-C message @@ -408,6 +423,9 @@ if (TernaryMiddle.isUsable()) TernaryMiddle = Actions.CorrectDelayedTyposInExpr(TernaryMiddle); LHS = ExprError(); + } else if (auto *B = dyn_cast(RHS.get())) { + if (B->isAssignmentOp()) + MarkNonConstUse(B->getRHS()); } NextTokPrec = getBinOpPrecedence(Tok.getKind(), GreaterThanIsOperator, Index: lib/Parse/ParseStmt.cpp =================================================================== --- lib/Parse/ParseStmt.cpp +++ lib/Parse/ParseStmt.cpp @@ -372,6 +372,87 @@ return Res; } +// Mark symbols in r-value expression as written. +void Parser::MarkNonConstUse(Expr *E) { + E = E->IgnoreParenCasts(); + if (auto *B = dyn_cast(E)) { + if (B->isAdditiveOp()) { + // p + 2 + MarkNonConstUse(B->getLHS()); + MarkNonConstUse(B->getRHS()); + } else if (B->isAssignmentOp()) { + // .. = p; + MarkNonConstUse(B->getRHS()); + } + } else if (auto *CE = dyn_cast(E)) { + // Mark nonconst function parameters as written. + const FunctionDecl *FD = CE->getDirectCallee(); + if (!FD) { + // Mark all arguments as written + // TODO: Handle CXXMemberCallExpr better + for (auto *Arg : CE->arguments()) { + MarkNonConstUse(Arg); + } + return; + } + unsigned ArgNr = 0U; + for (const auto *Par : FD->parameters()) { + if (ArgNr >= CE->getNumArgs()) + break; + // Is this a nonconstant pointer/reference parameter? + const Type *ParType = Par->getType().getTypePtr(); + if ((ParType->isPointerType() && !ParType->getPointeeType().isConstQualified()) || + (ParType->isReferenceType() && !Par->getType().isConstQualified())) { + // This is a nonconst pointer/reference parameter, the data is + // potentially written in the function. + Expr *Arg = CE->getArg(ArgNr); + if (ParType->isReferenceType()) + if (auto *U = dyn_cast(Arg)) + Arg = U->getSubExpr(); + MarkNonConstUse(Arg); + } + ArgNr++; + } + } else if (auto *C = dyn_cast(E)) { + MarkNonConstUse(C->getTrueExpr()); + MarkNonConstUse(C->getFalseExpr()); + } else if (auto *D = dyn_cast(E)) { + if (auto *VD = dyn_cast(D->getDecl())) + VD->setNonConstUse(); + } else if (isa(E)) { + // ++(*p); + bool IncDec = false; + bool Deref = false; + while (auto *U = dyn_cast(E)) { + switch (U->getOpcode()) { + case UO_PreInc: + case UO_PostInc: + case UO_PreDec: + case UO_PostDec: + IncDec = true; + break; + case UO_Deref: + if (IncDec) + Deref = true; + break; + default: + break; + }; + E = U->getSubExpr()->IgnoreParenCasts(); + } + if (auto *A = dyn_cast(E)) { + Deref = IncDec; + E = A->getBase()->IgnoreParenCasts(); + } + if (Deref) { + if (auto *D = dyn_cast(E)) { + if (auto *VD = dyn_cast(D->getDecl())) + VD->setNonConstUse(); + } + } + } +} + /// \brief Parse an expression statement. StmtResult Parser::ParseExprStatement() { // If a case keyword is missing, this is where it should be inserted. @@ -389,6 +470,9 @@ return Actions.ActOnExprStmtError(); } + // Mark symbols in the expression as written. + MarkNonConstUse(Expr.get()); + if (Tok.is(tok::colon) && getCurScope()->isSwitchScope() && Actions.CheckCaseExpression(Expr.get())) { // If a constant expression is followed by a colon inside a switch block, @@ -954,6 +1038,18 @@ StmtResult R; if (Tok.isNot(tok::kw___extension__)) { R = ParseStatementOrDeclaration(Stmts, false); + if (!R.isInvalid() && R.get()) { + if (ReturnStmt *RS = dyn_cast(R.get())) { + if (RS->getRetValue()) { + if (auto *Cast = dyn_cast(RS->getRetValue())) { + const Type *T = Cast->getType().getTypePtr(); + if (T->isPointerType() && !T->getPointeeType().isConstQualified()) { + MarkNonConstUse(Cast); + } + } + } + } + } } else { // __extension__ can start declarations and it can also be a unary // operator for expressions. Consume multiple __extension__ markers here Index: lib/Sema/SemaDecl.cpp =================================================================== --- lib/Sema/SemaDecl.cpp +++ lib/Sema/SemaDecl.cpp @@ -8975,6 +8975,24 @@ } } +static void MarkNonConstUse(Expr *E) { + E = E->IgnoreParenCasts(); + if (auto *B = dyn_cast(E)) { + if (B->isAdditiveOp()) { + MarkNonConstUse(B->getLHS()); + MarkNonConstUse(B->getRHS()); + } + } else if (auto *C = dyn_cast(E)) { + MarkNonConstUse(C->getFalseExpr()); + MarkNonConstUse(C->getTrueExpr()); + } else if (auto *D = dyn_cast(E)) { + if (auto *VD = dyn_cast(D->getDecl())) + VD->setNonConstUse(); + } else if (auto *U = dyn_cast(E)) { + MarkNonConstUse(U->getSubExpr()); + } +} + /// AddInitializerToDecl - Adds the initializer Init to the /// declaration dcl. If DirectInit is true, this is C++ direct /// initialization rather than copy initialization. @@ -8987,6 +9005,8 @@ return; } + MarkNonConstUse(Init); + if (CXXMethodDecl *Method = dyn_cast(RealDecl)) { // Pure-specifiers are handled in ActOnPureSpecifier. Diag(Method->getLocation(), diag::err_member_function_initialization) @@ -10398,6 +10418,41 @@ } } +void Sema::DiagnoseNonConstPointerParameters(ParmVarDecl * const *ParamBegin, + ParmVarDecl * const *ParamEnd) { + // Don't diagnose nonconst-parameter errors in template instantiations + if (!ActiveTemplateInstantiations.empty()) + return; + for (const auto *Param : llvm::make_range(ParamBegin, ParamEnd)) { + if (!Param->isReferenced() || Param->hasNonConstUse()) + continue; + const Type *T = Param->getType().getTypePtr(); + if (!T->isPointerType()) + continue; + if (T->getPointeeType().isConstQualified()) + continue; + // Don't warn about function pointers. + if (T->getPointeeType().getTypePtr()->isFunctionProtoType()) + continue; + // Don't warn about struct parameters. These are not handled properly by + // the code currently. There should not be warnings always when a struct + // pointer parameter can be const neither. + if (T->getPointeeType().getTypePtr()->isRecordType()) + continue; + // Don't warn about ** parameters. These are not handled properly by the + // code currently. + if (T->getPointeeType().getTypePtr()->isPointerType()) + continue; + // Don't warn about void * as such parameters are often used in casts that + // are hard to handle well. For now there are no warnings about such + // parameters. + if (T->getPointeeType().getTypePtr()->isVoidType()) + continue; + Diag(Param->getLocation(), diag::warn_nonconst_parameter) + << Param->getDeclName(); + } +} + void Sema::DiagnoseSizeOfParametersAndReturnValue(ParmVarDecl * const *Param, ParmVarDecl * const *ParamEnd, QualType ReturnTy, @@ -10970,6 +11025,8 @@ // Don't diagnose unused parameters of defaulted or deleted functions. if (!FD->isDeleted() && !FD->isDefaulted()) DiagnoseUnusedParameters(FD->param_begin(), FD->param_end()); + if (!getLangOpts().CPlusPlus) + DiagnoseNonConstPointerParameters(FD->param_begin(), FD->param_end()); DiagnoseSizeOfParametersAndReturnValue(FD->param_begin(), FD->param_end(), FD->getReturnType(), FD); @@ -10978,7 +11035,7 @@ MarkVTableUsed(FD->getLocation(), Constructor->getParent()); else if (CXXDestructorDecl *Destructor = dyn_cast(FD)) MarkVTableUsed(FD->getLocation(), Destructor->getParent()); - + // Try to apply the named return value optimization. We have to check // if we can do this here because lambdas keep return statements around // to deduce an implicit return type. Index: lib/Sema/SemaExpr.cpp =================================================================== --- lib/Sema/SemaExpr.cpp +++ lib/Sema/SemaExpr.cpp @@ -9492,6 +9492,19 @@ } } +/// \brief Set NonConstUse for variable declarations used in expression. +static void MarkNonConstUse(Expr *E) { + E = E->IgnoreParenCasts(); + if (auto *D = dyn_cast(E)) { + if (auto *VD = dyn_cast(D->getDecl())) + VD->setNonConstUse(); + } else if (auto *U = dyn_cast(E)) { + MarkNonConstUse(U->getSubExpr()); + } else if (auto *A = dyn_cast(E)) { + MarkNonConstUse(A->getBase()); + } +} + // C99 6.5.16.1 QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS, SourceLocation Loc, @@ -9502,6 +9515,8 @@ if (CheckForModifiableLvalue(LHSExpr, Loc, *this)) return QualType(); + MarkNonConstUse(LHSExpr); + QualType LHSType = LHSExpr->getType(); QualType RHSType = CompoundType.isNull() ? RHS.get()->getType() : CompoundType; @@ -12807,7 +12822,8 @@ CopyExpr = new (S.Context) DeclRefExpr(Var, RefersToCapturedVariable, DeclRefType, VK_LValue, Loc); - Var->setReferenced(true); + Var->setReferenced(); + Var->setNonConstUse(); Var->markUsed(S.Context); } Index: lib/Sema/SemaLambda.cpp =================================================================== --- lib/Sema/SemaLambda.cpp +++ lib/Sema/SemaLambda.cpp @@ -815,7 +815,8 @@ VarDecl *NewVD = VarDecl::Create(Context, CurContext, Loc, Loc, Id, InitCaptureType, TSI, SC_Auto); NewVD->setInitCapture(true); - NewVD->setReferenced(true); + NewVD->setReferenced(); + NewVD->setNonConstUse(); NewVD->markUsed(Context); NewVD->setInit(Init); return NewVD; Index: lib/Sema/SemaOpenMP.cpp =================================================================== --- lib/Sema/SemaOpenMP.cpp +++ lib/Sema/SemaOpenMP.cpp @@ -499,6 +499,7 @@ SourceLocation Loc, bool RefersToCapture = false) { D->setReferenced(); + D->setNonConstUse(); D->markUsed(S.Context); return DeclRefExpr::Create(S.getASTContext(), NestedNameSpecifierLoc(), SourceLocation(), D, RefersToCapture, Loc, Ty, Index: lib/Sema/SemaTemplateInstantiateDecl.cpp =================================================================== --- lib/Sema/SemaTemplateInstantiateDecl.cpp +++ lib/Sema/SemaTemplateInstantiateDecl.cpp @@ -3632,6 +3632,7 @@ if (OldVar->isUsed(false)) NewVar->setIsUsed(); NewVar->setReferenced(OldVar->isReferenced()); + NewVar->setNonConstUse(); } InstantiateAttrs(TemplateArgs, OldVar, NewVar, LateAttrs, StartingScope); Index: lib/Serialization/ASTReaderDecl.cpp =================================================================== --- lib/Serialization/ASTReaderDecl.cpp +++ lib/Serialization/ASTReaderDecl.cpp @@ -501,6 +501,8 @@ D->setImplicit(Record[Idx++]); D->Used = Record[Idx++]; D->setReferenced(Record[Idx++]); + if (auto *VD = dyn_cast(D)) + VD->setNonConstUse(); D->setTopLevelDeclInObjCContainer(Record[Idx++]); D->setAccess((AccessSpecifier)Record[Idx++]); D->FromASTFile = true; Index: test/Sema/warn-nonconst-parameter.c =================================================================== --- test/Sema/warn-nonconst-parameter.c +++ test/Sema/warn-nonconst-parameter.c @@ -0,0 +1,125 @@ +// RUN: %clang_cc1 -fsyntax-only -Wnonconst-parameter -verify %s + +// Currently the -Wnonconst-parameter only warns about pointer arguments. +// +// It can be defined both that the data is const and that the pointer is const, +// the -Wnonconst-parameter only checks if the data can be const-specified. +// +// It does not warn about pointers to records or function pointers. + +// Some external function where first argument is nonconst and second is const. +char* strcpy1(char *dest, const char *src); +unsigned my_strcpy(char *buf, const char *s); +unsigned my_strlen(const char *buf); + + +int warn1(int *p) { // expected-warning {{parameter 'p' can be const}} + return *p; +} + +void warn2(int *first, int *last) { // expected-warning {{parameter 'last' can be const}} + *first = 0; + if (first < last) {} // <- last can be const +} + +void warn3(char *p) { // expected-warning {{parameter 'p' can be const}} + char buf[10]; + strcpy1(buf, p); +} + +int dontwarn1(const int *p) { + return *p; +} + +void dontwarn2(int *p) { + *p = 0; +} + +void dontwarn3(char *p) { + p[0] = 0; +} + +void dontwarn4(int *p) { + int *q = p; + *q = 0; +} + +void dontwarn5(float *p) { + int *q = (int *)p; +} + +void dontwarn6(char *p) { + char *a, *b; + a = b = p; +} + +void dontwarn7(int *p) { + int *i = p ? p : 0; +} + +void dontwarn8(char *p) { + char *x; + x = (p ? p : ""); +} + +char *dontwarn9(char *p) { + char *x; + return p ? p : ""; +} + +void dontwarn10(int *p) { + ++(*p); +} + +char dontwarn11(int *p) { + return ++(*p); +} + +char *dontwarn12(char *s) { + return s; +} + +void dontwarn13(unsigned char *str, const unsigned int i) { + unsigned char *p; + for (p = str + i; *p; ) {} +} + +void dontwarn14(int *buf) { + int i, *p; + for (i=0, p=buf; i<10; i++, p++) { + *p = 1; + } +} + +void dontwarn15(int *p, int x) { + for (int *q = p+x-1; 0; q++); +} + +void dontwarn16(char *p) { + strcpy1(p, "abc"); +} + +void dontwarn17(char *p) { + strcpy1(p+2, "abc"); +} + + +char *dontwarn18(char *p) { + return strcpy1(p, "abc"); +} + +unsigned dontwarn19(char *buf) { + unsigned len = my_strlen(buf); + return len + my_strcpy(buf, "abc"); +} + +// Don't warn about nonconst function pointers that can be const. +void functionpointer(double f(double), int x) { + f(x); +} + +// Don't warn about nonconst record pointers that can be const. +struct XY { int x; int y; }; +int recordpointer(struct XY *xy) { + return xy->x; +} Index: test/SemaCXX/warn-nonconst-parameter.cpp =================================================================== --- test/SemaCXX/warn-nonconst-parameter.cpp +++ test/SemaCXX/warn-nonconst-parameter.cpp @@ -0,0 +1,44 @@ +// RUN: %clang_cc1 -fsyntax-only -Wnonconst-parameter -verify %s +// +// Test that -Wnonconst-parameter does not warn for virtual functions. +// + +// expected-no-diagnostics + +class Base { +public: + virtual int f(int*p); +}; + +class Derived : public Base { +public: + // Don't warn on inherited virtual function + virtual int f(int *p) { return 0; } + + // Don't warn on non-inherited virtual function + virtual int g(int *p) { return 0; } +}; + +class Fred { +private: + void *X; +public: + Fred(void *X) : X(X) {} + void dostuff(); +}; + +static Fred& dontwarn1(void *P) { + return *static_cast(P); +} + +static void dontwarn2(void *P) { + ((Fred*)P)->dostuff(); +} + + +// Don't warn when data is passed to nonconst reference parameter +void dostuff(int &x); +void dontwarn19(int *p) { + dostuff(*p); +} +