danielmarjamaki created this revision.
danielmarjamaki added a subscriber: cfe-commits.
This is a new warning for Clang. It will warn when a pointer parameter can be
const.
The design is inspired by the Wunused-parameter warning.
I have tested this on many debian packages. In 2151 projects there were 60834
warnings. So in average there were ~30 warnings / project. Most of my
"dontwarn" testcases are inspired by false positives I have seen and fixed. The
false positive ratio is imho not bad right now but I will continue to look in
the log to see if there are more false positives.
http://reviews.llvm.org/D12359
Files:
include/clang/AST/DeclBase.h
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
lib/Parse/ParseExpr.cpp
lib/Parse/ParseStmt.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaExpr.cpp
lib/Sema/SemaLambda.cpp
lib/Sema/SemaOpenMP.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
lib/Serialization/ASTReaderDecl.cpp
test/Sema/warn-nonconst-parameter.c
test/SemaCXX/warn-nonconst-parameter.cpp
Index: test/SemaCXX/warn-nonconst-parameter.cpp
===================================================================
--- test/SemaCXX/warn-nonconst-parameter.cpp
+++ test/SemaCXX/warn-nonconst-parameter.cpp
@@ -0,0 +1,17 @@
+// 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:
+ virtual int f(int*p){return 0;}
+};
Index: test/Sema/warn-nonconst-parameter.c
===================================================================
--- test/Sema/warn-nonconst-parameter.c
+++ test/Sema/warn-nonconst-parameter.c
@@ -0,0 +1,89 @@
+// 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.
+void strcpy1(char *dest, const char *src);
+
+
+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 : "");
+}
+
+void dontwarn9(int *p) {
+ ++(*p);
+}
+
+void dontwarn10(unsigned char *str, const unsigned int i) {
+ unsigned char *p;
+ for (p = str + i; *p; ) {}
+}
+
+void dontwarn11(char *p) {
+ strcpy1(p, "abc");
+}
+
+void dontwarn12(char *p) {
+ strcpy1(p+2, "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: lib/Serialization/ASTReaderDecl.cpp
===================================================================
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -500,6 +500,7 @@
D->setImplicit(Record[Idx++]);
D->Used = Record[Idx++];
D->setReferenced(Record[Idx++]);
+ D->setWritten();
D->setTopLevelDeclInObjCContainer(Record[Idx++]);
D->setAccess((AccessSpecifier)Record[Idx++]);
D->FromASTFile = true;
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===================================================================
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -3613,6 +3613,7 @@
if (OldVar->isUsed(false))
NewVar->setIsUsed();
NewVar->setReferenced(OldVar->isReferenced());
+ NewVar->setWritten();
}
// See if the old variable had a type-specifier that defined an anonymous tag.
Index: lib/Sema/SemaOpenMP.cpp
===================================================================
--- lib/Sema/SemaOpenMP.cpp
+++ lib/Sema/SemaOpenMP.cpp
@@ -466,6 +466,7 @@
SourceLocation Loc,
bool RefersToCapture = false) {
D->setReferenced();
+ D->setWritten();
D->markUsed(S.Context);
return DeclRefExpr::Create(S.getASTContext(), NestedNameSpecifierLoc(),
SourceLocation(), D, RefersToCapture, Loc, Ty,
Index: lib/Sema/SemaLambda.cpp
===================================================================
--- lib/Sema/SemaLambda.cpp
+++ lib/Sema/SemaLambda.cpp
@@ -814,7 +814,8 @@
VarDecl *NewVD = VarDecl::Create(Context, CurContext, Loc,
Loc, Id, InitCaptureType, TSI, SC_Auto);
NewVD->setInitCapture(true);
- NewVD->setReferenced(true);
+ NewVD->setReferenced();
+ NewVD->setWritten();
NewVD->markUsed(Context);
NewVD->setInit(Init);
return NewVD;
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -9503,6 +9503,17 @@
}
}
+// Mark symbols in l-value expression as written.
+static void MarkWritten(Expr *E) {
+ E = E->IgnoreParenCasts();
+ if (auto *D = dyn_cast<DeclRefExpr>(E))
+ D->getDecl()->setWritten();
+ else if (auto *U = dyn_cast<UnaryOperator>(E))
+ MarkWritten(U->getSubExpr());
+ else if (auto *A = dyn_cast<ArraySubscriptExpr>(E))
+ MarkWritten(A->getBase());
+}
+
// C99 6.5.16.1
QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS,
SourceLocation Loc,
@@ -9513,6 +9524,8 @@
if (CheckForModifiableLvalue(LHSExpr, Loc, *this))
return QualType();
+ MarkWritten(LHSExpr);
+
QualType LHSType = LHSExpr->getType();
QualType RHSType = CompoundType.isNull() ? RHS.get()->getType() :
CompoundType;
@@ -12782,7 +12795,8 @@
CopyExpr = new (S.Context) DeclRefExpr(Var, RefersToCapturedVariable,
DeclRefType, VK_LValue, Loc);
- Var->setReferenced(true);
+ Var->setReferenced();
+ Var->setWritten();
Var->markUsed(S.Context);
}
Index: lib/Sema/SemaDecl.cpp
===================================================================
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -8875,6 +8875,19 @@
}
}
+/// Mark symbols in init expression as written.
+static void MarkWritten(Expr *E) {
+ E = E->IgnoreParenCasts();
+ if (auto *C = dyn_cast<ConditionalOperator>(E)) {
+ MarkWritten(C->getFalseExpr());
+ MarkWritten(C->getTrueExpr());
+ } else if (auto *D = dyn_cast<DeclRefExpr>(E)) {
+ D->getDecl()->setWritten();
+ } else if (auto *U = dyn_cast<UnaryOperator>(E)) {
+ MarkWritten(U->getSubExpr());
+ }
+}
+
/// AddInitializerToDecl - Adds the initializer Init to the
/// declaration dcl. If DirectInit is true, this is C++ direct
/// initialization rather than copy initialization.
@@ -8887,6 +8900,8 @@
return;
}
+ MarkWritten(Init);
+
if (CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(RealDecl)) {
// Pure-specifiers are handled in ActOnPureSpecifier.
Diag(Method->getLocation(), diag::err_member_function_initialization)
@@ -10291,6 +10306,34 @@
}
}
+/// @brief Warn if a non-constant pointer parameter can be const.
+void Sema::DiagnoseNonConstParameters(ParmVarDecl *const *Param,
+ ParmVarDecl *const *ParamEnd) {
+ // Don't diagnose nonconst-parameter errors in template instantiations
+ if (!ActiveTemplateInstantiations.empty())
+ return;
+ for (; Param != ParamEnd; ++Param) {
+ if (!(*Param)->isReferenced() || (*Param)->isWritten())
+ 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 structs.
+ if (T->getPointeeType().getTypePtr()->isRecordType())
+ continue;
+ // Don't warn about **.
+ if (T->getPointeeType().getTypePtr()->isPointerType())
+ continue;
+ Diag((*Param)->getLocation(), diag::warn_nonconst_parameter)
+ << (*Param)->getDeclName();
+ }
+}
+
void Sema::DiagnoseSizeOfParametersAndReturnValue(ParmVarDecl * const *Param,
ParmVarDecl * const *ParamEnd,
QualType ReturnTy,
@@ -10863,6 +10906,7 @@
// Don't diagnose unused parameters of defaulted or deleted functions.
if (!FD->isDeleted() && !FD->isDefaulted())
DiagnoseUnusedParameters(FD->param_begin(), FD->param_end());
+ DiagnoseNonConstParameters(FD->param_begin(), FD->param_end());
DiagnoseSizeOfParametersAndReturnValue(FD->param_begin(), FD->param_end(),
FD->getReturnType(), FD);
Index: lib/Parse/ParseStmt.cpp
===================================================================
--- lib/Parse/ParseStmt.cpp
+++ lib/Parse/ParseStmt.cpp
@@ -372,6 +372,58 @@
return Res;
}
+// Mark symbols in r-value expression as written.
+static void MarkWritten(Expr *E) {
+ E = E->IgnoreParenCasts();
+ if (auto *B = dyn_cast<BinaryOperator>(E)) {
+ if (B->isAdditiveOp()) {
+ // p + 2
+ MarkWritten(B->getLHS());
+ MarkWritten(B->getRHS());
+ }
+ } else if (auto *CE = dyn_cast<CallExpr>(E)) {
+ // Mark nonconst function parameters as written.
+ for (auto *Arg : CE->arguments()) {
+ // Is this a nonconstant pointer argument?
+ const Type *T = Arg->getType().getTypePtr();
+ if (!T->isPointerType())
+ continue;
+ if (T->getPointeeType().isConstQualified())
+ continue;
+ // This is a nonconst pointer argument, the data is potentially written
+ // in the function.
+ MarkWritten(Arg);
+ }
+ } else if (auto *D = dyn_cast<DeclRefExpr>(E)) {
+ D->getDecl()->setWritten();
+ } else if (isa<UnaryOperator>(E)) {
+ // ++(*p);
+ bool IncDec = false;
+ bool Deref = false;
+ while (auto *U = dyn_cast<UnaryOperator>(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 (Deref) {
+ if (auto *D = dyn_cast<DeclRefExpr>(E))
+ D->getDecl()->setWritten();
+ }
+ }
+}
+
/// \brief Parse an expression statement.
StmtResult Parser::ParseExprStatement() {
// If a case keyword is missing, this is where it should be inserted.
@@ -389,6 +441,9 @@
return Actions.ActOnExprStmtError();
}
+ // Mark symbols in the expression as written.
+ MarkWritten(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,
Index: lib/Parse/ParseExpr.cpp
===================================================================
--- lib/Parse/ParseExpr.cpp
+++ lib/Parse/ParseExpr.cpp
@@ -153,6 +153,26 @@
return ParseRHSOfBinaryExpression(LHS, prec::Comma);
}
+// Mark symbols in expression as written.
+static void MarkWritten(Expr *E) {
+ if (!E)
+ return;
+ E = E->IgnoreParenCasts();
+ if (auto *B = dyn_cast<BinaryOperator>(E)) {
+ if (B->isAdditiveOp()) {
+ MarkWritten(B->getLHS());
+ MarkWritten(B->getRHS());
+ } else if (B->isAssignmentOp()) {
+ MarkWritten(B->getRHS());
+ }
+ } else if (auto *D = dyn_cast<DeclRefExpr>(E)) {
+ D->getDecl()->setWritten();
+ } else if (auto *C = dyn_cast<ConditionalOperator>(E)) {
+ MarkWritten(C->getTrueExpr());
+ MarkWritten(C->getFalseExpr());
+ }
+}
+
/// \brief Parse an expr that doesn't include (top-level) commas.
ExprResult Parser::ParseAssignmentExpression(TypeCastState isTypeCast) {
if (Tok.is(tok::code_completion)) {
@@ -167,7 +187,18 @@
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<BinaryOperator>(ER.get())) {
+ if (B->isAssignmentOp()) {
+ MarkWritten(B->getLHS());
+ MarkWritten(B->getRHS());
+ }
+ }
+ return ER;
}
/// \brief Parse an assignment expression where part of an Objective-C message
Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -1758,6 +1758,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 DiagnoseNonConstParameters(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: 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<UnusedParameter>, DefaultIgnore;
+def warn_nonconst_parameter : Warning<"parameter %0 can be const">,
+ InGroup<NonConstParameter>, DefaultIgnore;
def warn_unused_variable : Warning<"unused variable %0">,
InGroup<UnusedVariable>, DefaultIgnore;
def warn_unused_local_typedef : Warning<
Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -429,6 +429,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/AST/DeclBase.h
===================================================================
--- include/clang/AST/DeclBase.h
+++ include/clang/AST/DeclBase.h
@@ -273,6 +273,11 @@
/// are regarded as "referenced" but not "used".
unsigned Referenced : 1;
+ /// \brief Whether the declared symbol is written. Either directly or
+ /// indirectly. This makes it possible to see if a nonconst declaration can
+ /// be "const".
+ unsigned Written : 1;
+
/// \brief Whether statistic collection is enabled.
static bool StatisticsEnabled;
@@ -324,26 +329,25 @@
bool AccessDeclContextSanity() const;
protected:
-
Decl(Kind DK, DeclContext *DC, SourceLocation L)
- : NextInContextAndBits(), DeclCtx(DC),
- Loc(L), DeclKind(DK), InvalidDecl(0),
- HasAttrs(false), Implicit(false), Used(false), Referenced(false),
- Access(AS_none), FromASTFile(0), Hidden(DC && cast<Decl>(DC)->Hidden),
- IdentifierNamespace(getIdentifierNamespaceForKind(DK)),
- CacheValidAndLinkage(0)
- {
- if (StatisticsEnabled) add(DK);
+ : NextInContextAndBits(), DeclCtx(DC), Loc(L), DeclKind(DK),
+ InvalidDecl(0), HasAttrs(false), Implicit(false), Used(false),
+ Referenced(false), Written(false), Access(AS_none), FromASTFile(0),
+ Hidden(DC && cast<Decl>(DC)->Hidden),
+ IdentifierNamespace(getIdentifierNamespaceForKind(DK)),
+ CacheValidAndLinkage(0) {
+ if (StatisticsEnabled)
+ add(DK);
}
Decl(Kind DK, EmptyShell Empty)
- : NextInContextAndBits(), DeclKind(DK), InvalidDecl(0),
- HasAttrs(false), Implicit(false), Used(false), Referenced(false),
- Access(AS_none), FromASTFile(0), Hidden(0),
- IdentifierNamespace(getIdentifierNamespaceForKind(DK)),
- CacheValidAndLinkage(0)
- {
- if (StatisticsEnabled) add(DK);
+ : NextInContextAndBits(), DeclKind(DK), InvalidDecl(0), HasAttrs(false),
+ Implicit(false), Used(false), Referenced(false), Written(false),
+ Access(AS_none), FromASTFile(0), Hidden(0),
+ IdentifierNamespace(getIdentifierNamespaceForKind(DK)),
+ CacheValidAndLinkage(0) {
+ if (StatisticsEnabled)
+ add(DK);
}
virtual ~Decl();
@@ -538,6 +542,12 @@
void setReferenced(bool R = true) { Referenced = R; }
+ /// \brief Whether the declared symbol is written.
+ bool isWritten() const { return Written; }
+
+ /// \brief Set declaration as written.
+ void setWritten() { Written = Referenced; }
+
/// \brief Whether this declaration is a top-level declaration (function,
/// global variable, etc.) that is lexically inside an objc container
/// definition.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
