[clang-tidy] readability-non-const-parameter: add new check that warn…

…s when function parameters should be const

The check will warn when the constness will make the function interface safer.

Reviewers: alexfh

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D15332

llvm-svn: 279507

clang-tools-extra/clang-tidy/readability/CMakeLists.txt

@@ -12,6 +12,7 @@ add_clang_library(clangTidyReadabilityModule
   InconsistentDeclarationParameterNameCheck.cpp
   NamedParameterCheck.cpp
   NamespaceCommentCheck.cpp
+  NonConstParameterCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantControlFlowCheck.cpp
   RedundantStringCStrCheck.cpp

clang-tools-extra/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<ParmVarDecl>("Parm")) {
+    if (const DeclContext *D = Parm->getParentFunctionOrMethod()) {
+      if (const auto *M = dyn_cast<CXXMethodDecl>(D)) {
+        if (M->isVirtual() || M->size_overridden_methods() != 0)
+          return;
+      }
+    }
+    addParm(Parm);
+  } else if (const auto *Ctor =
+                 Result.Nodes.getNodeAs<CXXConstructorDecl>("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<DeclRefExpr>("Ref")) {
+    setReferenced(Ref);
+  } else if (const auto *S = Result.Nodes.getNodeAs<Stmt>("Mark")) {
+    if (const auto *B = dyn_cast<BinaryOperator>(S)) {
+      if (B->isAssignmentOp())
+        markCanNotBeConst(B, false);
+    } else if (const auto *CE = dyn_cast<CallExpr>(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<CXXConstructExpr>(S)) {
+      for (const auto *Arg : CE->arguments()) {
+        markCanNotBeConst(Arg->IgnoreParenCasts(), true);
+      }
+    } else if (const auto *R = dyn_cast<ReturnStmt>(S)) {
+      markCanNotBeConst(R->getRetValue(), true);
+    } else if (const auto *U = dyn_cast<UnaryOperator>(S)) {
+      markCanNotBeConst(U, true);
+    }
+  } else if (const auto *VD = Result.Nodes.getNodeAs<VarDecl>("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<ParmVarDecl>(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<ImplicitCastExpr>(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<BinaryOperator>(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<ConditionalOperator>(E)) {
+    markCanNotBeConst(C->getTrueExpr(), CanNotBeConst);
+    markCanNotBeConst(C->getFalseExpr(), CanNotBeConst);
+  } else if (const auto *U = dyn_cast<UnaryOperator>(E)) {
+    if (U->getOpcode() == UO_PreInc || U->getOpcode() == UO_PreDec ||
+        U->getOpcode() == UO_PostInc || U->getOpcode() == UO_PostDec) {
+      if (const auto *SubU =
+              dyn_cast<UnaryOperator>(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<ArraySubscriptExpr>(E)) {
+    markCanNotBeConst(A->getBase(), true);
+  } else if (const auto *CLE = dyn_cast<CompoundLiteralExpr>(E)) {
+    markCanNotBeConst(CLE->getInitializer(), true);
+  } else if (const auto *Constr = dyn_cast<CXXConstructExpr>(E)) {
+    for (const auto *Arg : Constr->arguments()) {
+      if (const auto *M = dyn_cast<MaterializeTemporaryExpr>(Arg))
+        markCanNotBeConst(cast<Expr>(M->getTemporary()), CanNotBeConst);
+    }
+  } else if (const auto *ILE = dyn_cast<InitListExpr>(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<DeclRefExpr>(E)) {
+      auto It = Parameters.find(dyn_cast<ParmVarDecl>(D->getDecl()));
+      if (It != Parameters.end())
+        It->second.CanBeConst = false;
+    }
+  }
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tools-extra/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<const ParmVarDecl *, ParmInfo> 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

clang-tools-extra/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 @@ class ReadabilityModule : public ClangTidyModule {
         "readability-static-definition-in-anonymous-namespace");
     CheckFactories.registerCheck<readability::NamedParameterCheck>(
         "readability-named-parameter");
+    CheckFactories.registerCheck<NonConstParameterCheck>(
+        "readability-non-const-parameter");
     CheckFactories.registerCheck<RedundantControlFlowCheck>(
         "readability-redundant-control-flow");
     CheckFactories.registerCheck<RedundantSmartptrGetCheck>(

clang-tools-extra/docs/ReleaseNotes.rst

@@ -85,6 +85,12 @@ Improvements to clang-tidy
   Warns about the performance overhead arising from concatenating strings using
   the ``operator+``, instead of ``operator+=``.
 
+- New `readability-non-const-parameter
+  <http://clang.llvm.org/extra/clang-tidy/checks/readability-non-const-parameter.html>`_ check
+
+  Flags function parameters of a pointer type that could be changed to point to
+  a constant type instead.
+
 Improvements to include-fixer
 -----------------------------
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -128,6 +128,7 @@ Clang-Tidy Checks
    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

clang-tools-extra/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;
+  }
+