Index: clang-tidy/readability/CMakeLists.txt =================================================================== --- clang-tidy/readability/CMakeLists.txt +++ clang-tidy/readability/CMakeLists.txt @@ -12,6 +12,7 @@ InconsistentDeclarationParameterNameCheck.cpp NamedParameterCheck.cpp NamespaceCommentCheck.cpp + NonConstParameterCheck.cpp ReadabilityTidyModule.cpp RedundantControlFlowCheck.cpp RedundantStringCStrCheck.cpp Index: clang-tidy/readability/NonConstParameterCheck.h =================================================================== --- clang-tidy/readability/NonConstParameterCheck.h +++ clang-tidy/readability/NonConstParameterCheck.h @@ -0,0 +1,64 @@ +//===--- NonConstParameterCheck.h - clang-tidy-------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_NON_CONST_PARAMETER_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_NON_CONST_PARAMETER_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace readability { + +/// Warn when a pointer function parameter can be const. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/readability-non-const-parameter.html +class NonConstParameterCheck : public ClangTidyCheck { +public: + NonConstParameterCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void onEndOfTranslationUnit() override; + +private: + /// Parameter info. + struct ParmInfo { + /// Is function parameter referenced? + bool IsReferenced; + + /// Can function parameter be const? + bool CanBeConst; + }; + + /// Track all nonconst integer/float parameters. + std::map Parameters; + + /// Add function parameter. + void addParm(const ParmVarDecl *Parm); + + /// Set IsReferenced. + void setReferenced(const DeclRefExpr *Ref); + + /// Set CanNotBeConst. + /// Visits sub expressions recursively. If a DeclRefExpr is found + /// and CanNotBeConst is true the Parameter is marked as not-const. + /// The CanNotBeConst is updated as sub expressions are visited. + void markCanNotBeConst(const Expr *E, bool CanNotBeConst); + + /// Diagnose non const parameters. + void diagnoseNonConstParameters(); +}; + +} // namespace readability +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_NON_CONST_PARAMETER_H Index: clang-tidy/readability/NonConstParameterCheck.cpp =================================================================== --- clang-tidy/readability/NonConstParameterCheck.cpp +++ clang-tidy/readability/NonConstParameterCheck.cpp @@ -0,0 +1,214 @@ +//===--- NonConstParameterCheck.cpp - clang-tidy---------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "NonConstParameterCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace readability { + +void NonConstParameterCheck::registerMatchers(MatchFinder *Finder) { + // Add parameters to Parameters. + Finder->addMatcher(parmVarDecl(unless(isInstantiated())).bind("Parm"), this); + + // C++ constructor. + Finder->addMatcher(cxxConstructorDecl().bind("Ctor"), this); + + // Track unused parameters, there is Wunused-parameter about unused + // parameters. + Finder->addMatcher(declRefExpr().bind("Ref"), this); + + // Analyse parameter usage in function. + Finder->addMatcher(stmt(anyOf(unaryOperator(anyOf(hasOperatorName("++"), + hasOperatorName("--"))), + binaryOperator(), callExpr(), returnStmt(), + cxxConstructExpr())) + .bind("Mark"), + this); + Finder->addMatcher(varDecl(hasInitializer(anything())).bind("Mark"), this); +} + +void NonConstParameterCheck::check(const MatchFinder::MatchResult &Result) { + if (const auto *Parm = Result.Nodes.getNodeAs("Parm")) { + if (const DeclContext *D = Parm->getParentFunctionOrMethod()) { + if (const auto *M = dyn_cast(D)) { + if (M->isVirtual() || M->size_overridden_methods() != 0) + return; + } + } + addParm(Parm); + } else if (const auto *Ctor = + Result.Nodes.getNodeAs("Ctor")) { + for (const auto *Parm : Ctor->parameters()) + addParm(Parm); + for (const auto *Init : Ctor->inits()) + markCanNotBeConst(Init->getInit(), true); + } else if (const auto *Ref = Result.Nodes.getNodeAs("Ref")) { + setReferenced(Ref); + } else if (const auto *S = Result.Nodes.getNodeAs("Mark")) { + if (const auto *B = dyn_cast(S)) { + if (B->isAssignmentOp()) + markCanNotBeConst(B, false); + } else if (const auto *CE = dyn_cast(S)) { + // Typically, if a parameter is const then it is fine to make the data + // const. But sometimes the data is written even though the parameter + // is const. Mark all data passed by address to the function. + for (const auto *Arg : CE->arguments()) { + markCanNotBeConst(Arg->IgnoreParenCasts(), true); + } + + // Data passed by nonconst reference should not be made const. + if (const FunctionDecl *FD = CE->getDirectCallee()) { + unsigned ArgNr = 0U; + for (const auto *Par : FD->parameters()) { + if (ArgNr >= CE->getNumArgs()) + break; + const Expr *Arg = CE->getArg(ArgNr++); + // Is this a non constant reference parameter? + const Type *ParType = Par->getType().getTypePtr(); + if (!ParType->isReferenceType() || Par->getType().isConstQualified()) + continue; + markCanNotBeConst(Arg->IgnoreParenCasts(), false); + } + } + } else if (const auto *CE = dyn_cast(S)) { + for (const auto *Arg : CE->arguments()) { + markCanNotBeConst(Arg->IgnoreParenCasts(), true); + } + } else if (const auto *R = dyn_cast(S)) { + markCanNotBeConst(R->getRetValue(), true); + } else if (const auto *U = dyn_cast(S)) { + markCanNotBeConst(U, true); + } + } else if (const auto *VD = Result.Nodes.getNodeAs("Mark")) { + const QualType T = VD->getType(); + if ((T->isPointerType() && !T->getPointeeType().isConstQualified()) || + T->isArrayType()) + markCanNotBeConst(VD->getInit(), true); + } +} + +void NonConstParameterCheck::addParm(const ParmVarDecl *Parm) { + // Only add nonconst integer/float pointer parameters. + const QualType T = Parm->getType(); + if (!T->isPointerType() || T->getPointeeType().isConstQualified() || + !(T->getPointeeType()->isIntegerType() || + T->getPointeeType()->isFloatingType())) + return; + + if (Parameters.find(Parm) != Parameters.end()) + return; + + ParmInfo PI; + PI.IsReferenced = false; + PI.CanBeConst = true; + Parameters[Parm] = PI; +} + +void NonConstParameterCheck::setReferenced(const DeclRefExpr *Ref) { + auto It = Parameters.find(dyn_cast(Ref->getDecl())); + if (It != Parameters.end()) + It->second.IsReferenced = true; +} + +void NonConstParameterCheck::onEndOfTranslationUnit() { + diagnoseNonConstParameters(); +} + +void NonConstParameterCheck::diagnoseNonConstParameters() { + for (const auto &It : Parameters) { + const ParmVarDecl *Par = It.first; + const ParmInfo &ParamInfo = It.second; + + // Unused parameter => there are other warnings about this. + if (!ParamInfo.IsReferenced) + continue; + + // Parameter can't be const. + if (!ParamInfo.CanBeConst) + continue; + + diag(Par->getLocation(), "pointer parameter '%0' can be pointer to const") + << Par->getName() + << FixItHint::CreateInsertion(Par->getLocStart(), "const "); + } +} + +void NonConstParameterCheck::markCanNotBeConst(const Expr *E, + bool CanNotBeConst) { + if (!E) + return; + + if (const auto *Cast = dyn_cast(E)) { + // If expression is const then ignore usage. + const QualType T = Cast->getType(); + if (T->isPointerType() && T->getPointeeType().isConstQualified()) + return; + } + + E = E->IgnoreParenCasts(); + + if (const auto *B = dyn_cast(E)) { + if (B->isAdditiveOp()) { + // p + 2 + markCanNotBeConst(B->getLHS(), CanNotBeConst); + markCanNotBeConst(B->getRHS(), CanNotBeConst); + } else if (B->isAssignmentOp()) { + markCanNotBeConst(B->getLHS(), false); + + // If LHS is not const then RHS can't be const. + const QualType T = B->getLHS()->getType(); + if (T->isPointerType() && !T->getPointeeType().isConstQualified()) + markCanNotBeConst(B->getRHS(), true); + } + } else if (const auto *C = dyn_cast(E)) { + markCanNotBeConst(C->getTrueExpr(), CanNotBeConst); + markCanNotBeConst(C->getFalseExpr(), CanNotBeConst); + } else if (const auto *U = dyn_cast(E)) { + if (U->getOpcode() == UO_PreInc || U->getOpcode() == UO_PreDec || + U->getOpcode() == UO_PostInc || U->getOpcode() == UO_PostDec) { + if (const auto *SubU = + dyn_cast(U->getSubExpr()->IgnoreParenCasts())) + markCanNotBeConst(SubU->getSubExpr(), true); + markCanNotBeConst(U->getSubExpr(), CanNotBeConst); + } else if (U->getOpcode() == UO_Deref) { + if (!CanNotBeConst) + markCanNotBeConst(U->getSubExpr(), true); + } else { + markCanNotBeConst(U->getSubExpr(), CanNotBeConst); + } + } else if (const auto *A = dyn_cast(E)) { + markCanNotBeConst(A->getBase(), true); + } else if (const auto *CLE = dyn_cast(E)) { + markCanNotBeConst(CLE->getInitializer(), true); + } else if (const auto *Constr = dyn_cast(E)) { + for (const auto *Arg : Constr->arguments()) { + if (const auto *M = dyn_cast(Arg)) + markCanNotBeConst(cast(M->getTemporary()), CanNotBeConst); + } + } else if (const auto *ILE = dyn_cast(E)) { + for (unsigned I = 0U; I < ILE->getNumInits(); ++I) + markCanNotBeConst(ILE->getInit(I), true); + } else if (CanNotBeConst) { + // Referencing parameter. + if (const auto *D = dyn_cast(E)) { + auto It = Parameters.find(dyn_cast(D->getDecl())); + if (It != Parameters.end()) + It->second.CanBeConst = false; + } + } +} + +} // namespace readability +} // namespace tidy +} // namespace clang Index: clang-tidy/readability/ReadabilityTidyModule.cpp =================================================================== --- clang-tidy/readability/ReadabilityTidyModule.cpp +++ clang-tidy/readability/ReadabilityTidyModule.cpp @@ -20,6 +20,7 @@ #include "ImplicitBoolCastCheck.h" #include "InconsistentDeclarationParameterNameCheck.h" #include "NamedParameterCheck.h" +#include "NonConstParameterCheck.h" #include "RedundantControlFlowCheck.h" #include "RedundantSmartptrGetCheck.h" #include "RedundantStringCStrCheck.h" @@ -57,6 +58,8 @@ "readability-static-definition-in-anonymous-namespace"); CheckFactories.registerCheck( "readability-named-parameter"); + CheckFactories.registerCheck( + "readability-non-const-parameter"); CheckFactories.registerCheck( "readability-redundant-control-flow"); CheckFactories.registerCheck( Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -85,6 +85,12 @@ Warns about the performance overhead arising from concatenating strings using the ``operator+``, instead of ``operator+=``. +- New `readability-non-const-parameter + `_ check + + Flags function parameters of a pointer type that could be changed to point to + a constant type instead. + Improvements to include-fixer ----------------------------- Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -128,6 +128,7 @@ readability-implicit-bool-cast readability-inconsistent-declaration-parameter-name readability-named-parameter + readability-non-const-parameter readability-redundant-control-flow readability-redundant-smartptr-get readability-redundant-string-cstr Index: docs/clang-tidy/checks/readability-non-const-parameter.rst =================================================================== --- docs/clang-tidy/checks/readability-non-const-parameter.rst +++ docs/clang-tidy/checks/readability-non-const-parameter.rst @@ -0,0 +1,44 @@ +.. title:: clang-tidy - readability-non-const-parameter + +readability-non-const-parameter +=============================== + +The check finds function parameters of a pointer type that could be changed to +point to a constant type instead. + +When const is used properly, many mistakes can be avoided. Advantages when using +const properly: + * prevent unintentional modification of data + * get additional warnings such as using uninitialized data + * make it easier for developers to see possible side effects + +This check is not strict about constness, it only warns when the constness will +make the function interface safer. + +.. code:: c++ + + // warning here; the declaration "const char *p" would make the function + // interface safer. + char f1(char *p) { + return *p; + } + + // no warning; the declaration could be more const "const int * const p" but + // that does not make the function interface safer. + int f2(const int *p) { + return *p; + } + + // no warning; making x const does not make the function interface safer + int f3(int x) { + return x; + } + + // no warning; Technically, *p can be const ("const struct S *p"). But making + // *p const could be misleading. People might think that it's safe to pass + // const data to this function. + struct S { int *a; int *b; }; + int f3(struct S *p) { + *(p->a) = 0; + } + Index: test/clang-tidy/readability-non-const-parameter.cpp =================================================================== --- test/clang-tidy/readability-non-const-parameter.cpp +++ test/clang-tidy/readability-non-const-parameter.cpp @@ -0,0 +1,279 @@ +// RUN: %check_clang_tidy %s readability-non-const-parameter %t + +// Currently the checker only warns about pointer arguments. +// +// It can be defined both that the data is const and that the pointer is const, +// the checker 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); + +// CHECK-MESSAGES: :[[@LINE+1]]:29: warning: pointer parameter 'last' can be pointer to const [readability-non-const-parameter] +void warn1(int *first, int *last) { + // CHECK-FIXES: {{^}}void warn1(int *first, const int *last) {{{$}} + *first = 0; + if (first < last) { + } // <- last can be const +} + +// TODO: warning should be written here +void warn2(char *p) { + char buf[10]; + strcpy1(buf, p); +} + +// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: pointer parameter 'p' can be +void assign1(int *p) { + // CHECK-FIXES: {{^}}void assign1(const int *p) {{{$}} + const int *q; + q = p; +} + +// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: pointer parameter 'p' can be +void assign2(int *p) { + // CHECK-FIXES: {{^}}void assign2(const int *p) {{{$}} + const int *q; + q = p + 1; +} + +void assign3(int *p) { + *p = 0; +} + +void assign4(int *p) { + *p += 2; +} + +void assign5(char *p) { + p[0] = 0; +} + +void assign6(int *p) { + int *q; + q = p++; +} + +void assign7(char *p) { + char *a, *b; + a = b = p; +} + +void assign8(char *a, char *b) { + char *x; + x = (a ? a : b); +} + +void assign9(unsigned char *str, const unsigned int i) { + unsigned char *p; + for (p = str + i; *p;) { + } +} + +void assign10(int *buf) { + int i, *p; + for (i = 0, p = buf; i < 10; i++, p++) { + *p = 1; + } +} + +// CHECK-MESSAGES: :[[@LINE+1]]:17: warning: pointer parameter 'p' can be +void init1(int *p) { + // CHECK-FIXES: {{^}}void init1(const int *p) {{{$}} + const int *q = p; +} + +// CHECK-MESSAGES: :[[@LINE+1]]:17: warning: pointer parameter 'p' can be +void init2(int *p) { + // CHECK-FIXES: {{^}}void init2(const int *p) {{{$}} + const int *q = p + 1; +} + +void init3(int *p) { + int *q = p; +} + +void init4(float *p) { + int *q = (int *)p; +} + +void init5(int *p) { + int *i = p ? p : 0; +} + +void init6(int *p) { + int *a[] = {p, p, 0}; +} + +void init7(int *p, int x) { + for (int *q = p + x - 1; 0; q++) + ; +} + +// CHECK-MESSAGES: :[[@LINE+1]]:18: warning: pointer parameter 'p' can be +int return1(int *p) { + // CHECK-FIXES: {{^}}int return1(const int *p) {{{$}} + return *p; +} + +// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: pointer parameter 'p' can be +const int *return2(int *p) { + // CHECK-FIXES: {{^}}const int *return2(const int *p) {{{$}} + return p; +} + +// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: pointer parameter 'p' can be +const int *return3(int *p) { + // CHECK-FIXES: {{^}}const int *return3(const int *p) {{{$}} + return p + 1; +} + +// CHECK-MESSAGES: :[[@LINE+1]]:27: warning: pointer parameter 'p' can be +const char *return4(char *p) { + // CHECK-FIXES: {{^}}const char *return4(const char *p) {{{$}} + return p ? p : ""; +} + +char *return5(char *s) { + return s; +} + +char *return6(char *s) { + return s + 1; +} + +char *return7(char *a, char *b) { + return a ? a : b; +} + +char return8(int *p) { + return ++(*p); +} + +void dontwarn1(int *p) { + ++(*p); +} + +void dontwarn2(int *p) { + (*p)++; +} + +int dontwarn3(_Atomic(int) * p) { + return *p; +} + +void callFunction1(char *p) { + strcpy1(p, "abc"); +} + +void callFunction2(char *p) { + strcpy1(&p[0], "abc"); +} + +void callFunction3(char *p) { + strcpy1(p + 2, "abc"); +} + +char *callFunction4(char *p) { + return strcpy1(p, "abc"); +} + +unsigned callFunction5(char *buf) { + unsigned len = my_strlen(buf); + return len + my_strcpy(buf, "abc"); +} + +void f6(int **p); +void callFunction6(int *p) { f6(&p); } + +typedef union { void *v; } t; +void f7(t obj); +void callFunction7(int *p) { + f7((t){p}); +} + +void f8(int &x); +void callFunction8(int *p) { + f8(*p); +} + +// Don't warn about nonconst function pointers that can be const. +void functionpointer(double f(double), int x) { + f(x); +} + +// TODO: This is a false positive. +// CHECK-MESSAGES: :[[@LINE+1]]:27: warning: pointer parameter 'p' can be +int functionpointer2(int *p) { + return *p; +} +void use_functionpointer2() { + int (*fp)(int *) = functionpointer2; // <- the parameter 'p' can't be const +} + +// Don't warn about nonconst record pointers that can be const. +struct XY { + int *x; + int *y; +}; +void recordpointer(struct XY *xy) { + *(xy->x) = 0; +} + +class C { +public: + C(int *p) : p(p) {} + +private: + int *p; +}; + +class C2 { +public: + // CHECK-MESSAGES: :[[@LINE+1]]:11: warning: pointer parameter 'p' can be + C2(int *p) : p(p) {} + // CHECK-FIXES: {{^}} C2(const int *p) : p(p) {}{{$}} + +private: + const int *p; +}; + +void tempObject(int *p) { + C c(p); +} + +// avoid fp for const pointer array +void constPointerArray(const char *remapped[][2]) { + const char *name = remapped[0][0]; +} + +class Warn { +public: + // CHECK-MESSAGES: :[[@LINE+1]]:21: warning: pointer parameter 'p' can be + void doStuff(int *p) { + // CHECK-FIXES: {{^}} void doStuff(const int *p) {{{$}} + x = *p; + } + +private: + int x; +}; + +class Base { +public: + // Ensure there is no false positive for this method. It is virtual. + virtual void doStuff(int *p) { + int x = *p; + } +}; + +class Derived : public Base { +public: + // Ensure there is no false positive for this method. It overrides a method. + void doStuff(int *p) override { + int x = *p; + } +};