danielmarjamaki updated this revision to Diff 35482.
danielmarjamaki marked 9 inline comments as done.
danielmarjamaki added a comment.
With the previous patch there was much noise when building Clang. I have fixed
many false positives with improved handling of C++ code. However I have not
been able to properly handle templates yet.
With this patch, no -Wnonconst-parameter diagnostics are written for C++ code.
I hope that it will be possible to fix the noise for C++ code later and enable
this diagnostic for C++ code also.
http://reviews.llvm.org/D12359
Files:
include/clang/AST/DeclBase.h
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Parse/Parser.h
include/clang/Sema/Sema.h
lib/Parse/ParseDeclCXX.cpp
lib/Parse/ParseExpr.cpp
lib/Parse/ParseExprCXX.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
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
@@ -3631,6 +3631,8 @@
if (OldVar->isUsed(false))
NewVar->setIsUsed();
NewVar->setReferenced(OldVar->isReferenced());
+ if (OldVar->isWritten())
+ NewVar->setWritten();
}
InstantiateAttrs(TemplateArgs, OldVar, NewVar, LateAttrs, StartingScope);
Index: lib/Sema/SemaOpenMP.cpp
===================================================================
--- lib/Sema/SemaOpenMP.cpp
+++ lib/Sema/SemaOpenMP.cpp
@@ -482,6 +482,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
@@ -9525,6 +9525,17 @@
}
}
+/// \brief 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,
@@ -9535,6 +9546,8 @@
if (CheckForModifiableLvalue(LHSExpr, Loc, *this))
return QualType();
+ MarkWritten(LHSExpr);
+
QualType LHSType = LHSExpr->getType();
QualType RHSType = CompoundType.isNull() ? RHS.get()->getType() :
CompoundType;
@@ -12818,7 +12831,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
@@ -8931,6 +8931,23 @@
}
}
+static void MarkWritten(Expr *E) {
+ E = E->IgnoreParenCasts();
+ if (auto *B = dyn_cast<BinaryOperator>(E)) {
+ if (B->isAdditiveOp()) {
+ MarkWritten(B->getLHS());
+ MarkWritten(B->getRHS());
+ }
+ } else 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.
@@ -8943,6 +8960,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)
@@ -10356,6 +10375,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->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 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,
@@ -10928,15 +10982,17 @@
// 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);
// If this is a structor, we need a vtable.
if (CXXConstructorDecl *Constructor = dyn_cast<CXXConstructorDecl>(FD))
MarkVTableUsed(FD->getLocation(), Constructor->getParent());
else if (CXXDestructorDecl *Destructor = dyn_cast<CXXDestructorDecl>(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/Parse/ParseStmt.cpp
===================================================================
--- lib/Parse/ParseStmt.cpp
+++ lib/Parse/ParseStmt.cpp
@@ -372,6 +372,84 @@
return Res;
}
+// Mark symbols in r-value expression as written.
+void Parser::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 (B->isAssignmentOp()) {
+ // .. = p;
+ MarkWritten(B->getRHS());
+ }
+ } else if (auto *CE = dyn_cast<CallExpr>(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()) {
+ MarkWritten(Arg);
+ }
+ return;
+ }
+ int ArgNr = -1;
+ for (const auto *Par : FD->parameters()) {
+ ArgNr++;
+ if ((unsigned)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((unsigned)ArgNr);
+ if (ParType->isReferenceType())
+ if (auto *U = dyn_cast<UnaryOperator>(Arg))
+ Arg = U->getSubExpr();
+ MarkWritten(Arg);
+ }
+ }
+ } else if (auto *C = dyn_cast<ConditionalOperator>(E)) {
+ MarkWritten(C->getTrueExpr());
+ MarkWritten(C->getFalseExpr());
+ } 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 (auto *A = dyn_cast<ArraySubscriptExpr>(E)) {
+ Deref = IncDec;
+ E = A->getBase()->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 +467,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,
@@ -954,6 +1035,18 @@
StmtResult R;
if (Tok.isNot(tok::kw___extension__)) {
R = ParseStatementOrDeclaration(Stmts, false);
+ if (!R.isInvalid() && R.get()) {
+ if (ReturnStmt *RS = dyn_cast<ReturnStmt>(R.get())) {
+ if (RS->getRetValue()) {
+ if (auto *Cast = dyn_cast<ImplicitCastExpr>(RS->getRetValue())) {
+ const Type *T = Cast->getType().getTypePtr();
+ if (T->isPointerType() && !T->getPointeeType().isConstQualified()) {
+ MarkWritten(Cast);
+ }
+ }
+ }
+ }
+ }
} else {
// __extension__ can start declarations and it can also be a unary
// operator for expressions. Consume multiple __extension__ markers here
Index: lib/Parse/ParseExprCXX.cpp
===================================================================
--- lib/Parse/ParseExprCXX.cpp
+++ lib/Parse/ParseExprCXX.cpp
@@ -2828,6 +2828,8 @@
if (Operand.isInvalid())
return Operand;
+ MarkWritten(Operand.get());
+
return Actions.ActOnCXXDelete(Start, UseGlobal, ArrayDelete, Operand.get());
}
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<BinaryOperator>(ER.get())) {
+ if (B->isAssignmentOp() || B->isAdditiveOp()) {
+ MarkWritten(B->getLHS());
+ MarkWritten(B->getRHS());
+ }
+ } else if (isa<CallExpr>(ER.get()) ||
+ isa<ConditionalOperator>(ER.get()) ||
+ isa<UnaryOperator>(ER.get())) {
+ MarkWritten(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<BinaryOperator>(RHS.get())) {
+ if (B->isAssignmentOp())
+ MarkWritten(B->getRHS());
}
NextTokPrec = getBinOpPrecedence(Tok.getKind(), GreaterThanIsOperator,
Index: lib/Parse/ParseDeclCXX.cpp
===================================================================
--- lib/Parse/ParseDeclCXX.cpp
+++ lib/Parse/ParseDeclCXX.cpp
@@ -3285,6 +3285,12 @@
return true;
}
+ // Mark initializer parameters as written.
+ for (Expr *E : ArgExprs) {
+ if (auto *D = dyn_cast<DeclRefExpr>(E))
+ D->getDecl()->setWritten();
+ }
+
T.consumeClose();
SourceLocation EllipsisLoc;
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: include/clang/Parse/Parser.h
===================================================================
--- include/clang/Parse/Parser.h
+++ include/clang/Parse/Parser.h
@@ -1394,6 +1394,9 @@
bool IsUnevaluated);
private:
+ /// \brief Mark symbols in expression as written.
+ static void MarkWritten(Expr *E);
+
ExprResult ParseExpressionWithLeadingAt(SourceLocation AtLoc);
ExprResult ParseExpressionWithLeadingExtension(SourceLocation ExtLoc);
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
@@ -431,6 +431,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,13 @@
void setReferenced(bool R = true) { Referenced = R; }
+ /// \brief Whether the declared symbol is written either directly or
+ /// indirectly. A "written" declaration can't be const.
+ 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
