new clang-tidy checker readability-non-const-parameter

[danielmarjamaki]

Summary

This is a new clang-tidy checker that will warn when function parameters should be const.

See also related discussions in http://reviews.llvm.org/D12359

As usual, I am testing this checker on the debian projects. I have so far triaged 20 warnings from this clang-tidy checker and they were all true positives.

[aaron.ballman]

clang-tidy/readability/NonConstParameterCheck.cpp

43 ↗

(On Diff #42168)

I think it may be an improvement to unify all of the matchers that will eventually be used to call markCanNotBeConst() from check() into a single matcher. e.g.,

Finder->addMatcher(
  anyOf(stmt(anyOf(binaryOperator(...), callExpr(), returnStmt(), unaryOperator(...))).bind("mark"),
             varDecl(...).bind("mark")), this);

The implementation of check() won't have to change too much, but this reduces the number of individual matchers required which I think may be a performance win (gut feeling, not based on evidence) and does not reduce clarity.

48 ↗

(On Diff #42168)

What will this do with code like:

int f(int *p) {
  void g();
  return *p;
}

or

int f(int *p) {
  struct S {
    void g();
  };
  return *p;
}

63 ↗

(On Diff #42168)

Better how?

89 ↗

(On Diff #42168)

This loop can be replaced with std::find_if().

124 ↗

(On Diff #42168)

These should all be const auto * because E is a const Expr *.

clang-tidy/readability/NonConstParameterCheck.h

19 ↗

(On Diff #42168)

I think this struct can be local to NonConstParameterCheck.

43 ↗

(On Diff #42168)

No need for the struct keyword, here and elsewhere.

44 ↗

(On Diff #42168)

addParm instead of addPar, for clarity?

45 ↗

(On Diff #42168)

I haven't gotten to the function definition yet, but I have no idea what this function would do. ;-) A more clear identifier would be appreciated.

[alexfh]

clang-tidy/readability/NonConstParameterCheck.cpp

20 ↗

(On Diff #42168)

I'd prefer the problem to be fixed right away instead of disabling this check for C++. The specific issue of parameters of template functions can probably be addressed by disabling matches in template instantiations (add unless(isInTemplateInstantiation()) or unless(isInstantiated()) depending on what kind of node is being matched).

83 ↗

(On Diff #42168)

Use the if (T *x = ...) {...} pattern, please.

89 ↗

(On Diff #42168)

While I agree with similar comments in general, it seems that here it wouldn't make the code any better.

168 ↗

(On Diff #42168)

if (ParInfo *PI = ...) ...

docs/clang-tidy/checks/readability-non-const-parameter.rst

14 ↗

(On Diff #42168)

Let's format the code examples in LLVM style.

test/clang-tidy/readability-non-const-parameter.c

211 ↗

(On Diff #42168)

clang-format?

[danielmarjamaki]

I have tried to fix the comments.

This patch is mostly untested. I have only run 'make check-all'. I am running more tests right now.

[danielmarjamaki]

clang-tidy/readability/NonConstParameterCheck.cpp

44 ↗

(On Diff #42292)

I tried to unify like that. It almost worked. I failed to put the stmt(...) and varDecl(...) in the same matcher.

49 ↗

(On Diff #42292)

good question!

I tested and the checker seemed to handle these. apparently the ast-matcher scanned the functions recursively. However I don't know if we know that it will work, so I changed the checker.

84 ↗

(On Diff #42292)

I believe I fixed all these.

[alexfh]

Please add tests with templates and macros. Please also run this on LLVM/Clang to ensure a) it doesn't crash; b) the number of warnings is sane; c) a representative sample of warnings doesn't contain false positives.

[danielmarjamaki]

In D15332#305976, @alexfh wrote:

Please add tests with templates and macros. Please also run this on LLVM/Clang to ensure a) it doesn't crash; b) the number of warnings is sane; c) a representative sample of warnings doesn't contain false positives.

I am running clang-tidy on LLVM/Clang right now. I expect that it will take several hours.

I expect that I will get false positives when there are templates and I will add corresponding test cases.

[Eugene.Zelenko]

This check partially implements PR19419. Could it be extended to variables?

[danielmarjamaki]

In D15332#306097, @Eugene.Zelenko wrote:

This check partially implements PR19419. Could it be extended to variables?

Yes, I don't see any technical reasons why that would not work. However politically, some people like const variables and others don't.

[Eugene.Zelenko]

In D15332#306831, @danielmarjamaki wrote:

In D15332#306097, @Eugene.Zelenko wrote:

This check partially implements PR19419. Could it be extended to variables?

Yes, I don't see any technical reasons why that would not work. However politically, some people like const variables and others don't.

May be implement as separate check with some code sharing if applicable?

[danielmarjamaki]

Run this check on C++ code also. I have rerun the add_new_check python script. Minor code cleanups.

I have run this on the debian packages again. In 1806 projects there were 9691 warnings. I have so far triaged 51 of those warnings and they were all true positives as far as I saw.

I see no warning when running on Clang source code (files in llvm/tools/clang/lib/...).

[danielmarjamaki]

In D15332#352556, @danielmarjamaki wrote:

I see no warning when running on Clang source code (files in llvm/tools/clang/lib/...).

For information I rerun with --header-filter=* and got some results.. I will triage those..

[LegalizeAdulthood]

docs/clang-tidy/checks/readability-non-const-parameter.rst

15 ↗

(On Diff #47962)

With pointers, there are always two layers of constness:

• Whether the pointer itself is modified
• Whether the data pointed to is modified

When you say "p should be const", I read that as the former.

I am pretty sure you intended the latter. You should be more explicit in your documentation. The ambiguity would have been resolved if you showed the "after" version of the code that would eliminate the warning. This is semi-implied by your next example, but it's kinder to the reader to resolve this ambiguity immediately.

32 ↗

(On Diff #47962)

Here you are using "making p const" in the first sense I noted above.

It might help if you say "make *p const" when you mean that p points to constant data and use "making p const" when you mean that the pointer itself is unmodified.

There is also the wart in C++ that one can declare a function like this:

int f(char *p);

and define it like this:

int f(char *const p) {
  // ...
}

My sense is that this is pretty confusing to most programmers who really want the declaration and definition to be textually identical.

[LegalizeAdulthood]

test/clang-tidy/readability-non-const-parameter.cpp

3 ↗

(On Diff #47962)

How hard is it to extend it to references?

Certainly the confusion about what is const is easier to resolve in the case of references because the references themselves are immutable.

[danielmarjamaki]

docs/clang-tidy/checks/readability-non-const-parameter.rst

15 ↗

(On Diff #47962)

ok I understand, will try to improve

test/clang-tidy/readability-non-const-parameter.cpp

3 ↗

(On Diff #47962)

If a "int &" reference parameter is not written then probably it's better to pass it by value than making it const. I would prefer that unless it has to use "int &" to comply with some interface.

I realize that the same can be said about pointers. If there is a "int *p" and you just read the value that p points at .. maybe sometimes it would be preferable to pass it by value.

[LegalizeAdulthood]

test/clang-tidy/readability-non-const-parameter.cpp

3 ↗

(On Diff #47962)

I thought the point of this check was to identify stuff passed by reference/pointer that could instead be passed by const reference/pointer.

Passing a read-only number type (short, char, int, float, double, etc.) parameter by pointer or by reference/pointer is a code smell, but this check isn't trying to identify that situation.

I'm more interested in things like:

void foo(std::string &s);

becoming

void foo(const std::string &s);

when s is treated as a read-only value within the implementation of foo.

[danielmarjamaki]

test/clang-tidy/readability-non-const-parameter.cpp

3 ↗

(On Diff #47962)

I agree pointers and references are related from user perspective.

But technically I think we can't reuse the same markCanNotBeConst code.

This expression "p += 2" does not prevent that *p is const. But "r += 2" would prevent that the reference r is const.

I'm more interested in things like:

Yes that would be interesting. However I only warn about number types right now. As my last example in the rst file shows I don't warn for non-constant struct pointers for a reason.

Imho, I would like to have a warning when the data is modified even though *p is const. Like this:

struct S { int *a; int *b; };
int f3(const struct S * const p) {
  *(p->a) = 0; // <- I would like to have warning here
}

Also dealing with C++ is not my priority. I write this check so I can use it on C code.

[danielmarjamaki]

Updated documentation

[danielmarjamaki]

docs/clang-tidy/checks/readability-non-const-parameter.rst

16 ↗

(On Diff #50032)

thanks. I have tried to make it more clear in latest patch.

33 ↗

(On Diff #50032)

thanks. fixed in latest patch.

[danielmarjamaki]

test/clang-tidy/readability-non-const-parameter.cpp

4 ↗

(On Diff #50032)

Imho, I would like to have a warning when the data is modified even though *p is const. Like this:

hmm.. sorry about that.. that is of course out-of-scope for this check so not relevant to discuss here.

[danielmarjamaki]

ping.

[hokein]

clang-tidy/readability/NonConstParameterCheck.cpp

24 ↗

(On Diff #50032)

Extra . at the end.

124 ↗

(On Diff #50032)

const auto &It

clang-tidy/readability/NonConstParameterCheck.h

53 ↗

(On Diff #50032)

s/are/is

docs/clang-tidy/checks/readability-non-const-parameter.rst

6 ↗

(On Diff #50032)

Looks like what the document says isn't consistent with the check, since the check only finds non-const pointer parameter.

test/clang-tidy/readability-non-const-parameter.cpp

3 ↗

(On Diff #50032)

It makes sense to move this document to the rst, I think.

15 ↗

(On Diff #50032)

Just keep the whole warning message in the first CHECK-MESSAGE.
And remove [readability-non-const-parameter] in others CHECK-MESSAGE for keeping the line short.

219 ↗

(On Diff #50032)

Please add a test case:

class C {
public:
  C(int *p) : p(p) {}
private:
  const int *p;
};

BTW, does the check support class method?

[danielmarjamaki]

docs/clang-tidy/checks/readability-non-const-parameter.rst

6 ↗

(On Diff #50032)

I changed "Finds function parameters.." to "Finds function pointer parameters..".

test/clang-tidy/readability-non-const-parameter.cpp

3 ↗

(On Diff #50032)

Done. First line in rst will now say: "Finds function pointer parameters that should be const".

219 ↗

(On Diff #50032)

Thanks, I added such class.

I also added one class that shows that class methods are supported.

[danielmarjamaki]

Fixed review comments

[rsmith]

test/clang-tidy/readability-non-const-parameter.cpp

116–134 ↗

(On Diff #50705)

The wording of this warning, and the name of the check, are highly misleading.

It's *not* the parameter that can be const, it's the parameter's *pointee*.

[danielmarjamaki]

test/clang-tidy/readability-non-const-parameter.cpp

116–134 ↗

(On Diff #50705)

I understand. Figuring out a better message is not easy. I and a collegue brain stormed:

pointer parameter 'p' could be declared with const 'const int *p'

pointer parameter 'p' could be pointer to const

pointer parameter 'p' declaration could be pointer to const

missing const in pointer parameter 'p' declaration

declaration of pointer parameter 'p' could be 'const int *p'

Do you think any of these would be good?

[LegalizeAdulthood]

test/clang-tidy/readability-non-const-parameter.cpp

116–134 ↗

(On Diff #50705)

Regarding the name of the check, I can't think of anything better only things that are longer but aren't substantially more clear in revealing the intent of the check. IMO, you don't blindly run clang-tidy checks based on the name, you run them after reading what they do and deciding if you want that transformation or not.

In order of my preference:

• pointer parameter 'p' could be pointer to const
• declaration of pointer parameter 'p' could be 'const int *p'

I might use the word 'can' instead of the world 'could', but that's a very minor quibble.

[alexfh]

clang-tidy/readability/NonConstParameterCheck.cpp

44 ↗

(On Diff #50705)

const auto *Ctor

docs/clang-tidy/checks/readability-non-const-parameter.rst

7 ↗

(On Diff #50705)

That now sounds ambiguous (is it about parameters of a pointer to a function type?). I'd say "The check finds function parameters of a pointer type that could be changed to point to a constant type instead." or something similar.

8 ↗

(On Diff #50705)

"prevent unintentional modification of data"

10 ↗

(On Diff #50705)

"make it easier for developers ..."

12 ↗

(On Diff #50705)

s/clang-tidy/This check/

test/clang-tidy/readability-non-const-parameter.cpp

244 ↗

(On Diff #50705)

Please add a test to ensure that overridden methods are not modified (since you would need to update the base class' method and then all the other overrides.

251 ↗

(On Diff #50705)

The check can break code taking a pointer to a function it modifies:

typedef int (*fn)(int *);
int f(int *p) {return *p; }
fn fp = &f;

It's not always possible to detect, when this happens (for example, a pointer to the function could be taken in a different translation unit), but we should at least keep this case in mind. Please add a test case for this.

[Eugene.Zelenko]

Please also mention check in docs/ReleaseNotes.rst.

docs/clang-tidy/checks/readability-non-const-parameter.rst

15 ↗

(On Diff #50705)

I think you should add ".. code-block:: c++" before code block.

[danielmarjamaki]

Fix patch so it can be applied in latest trunk. Tried to fix review comments.

[danielmarjamaki]

test/clang-tidy/readability-non-const-parameter.cpp

117–135 ↗

(On Diff #67500)

I have changed the message now. pointer parameter 'p' can be pointer to const

The name is the same. Do you think it would be better with PointerParameterConstnessCheck() perhaps?

[danielmarjamaki]

fixed more review comments

[danielmarjamaki]

test/clang-tidy/readability-non-const-parameter.cpp

245 ↗

(On Diff #67504)

I have added tests and fixed FPs in latest diff.

252 ↗

(On Diff #67504)

I have added a test in latest diff. but there is a false positive. I believe I heard about whole program analysis in clang-tidy sometime.. is that something worked on / implemented yet?

[danielmarjamaki]

added this check to the release notes. run clang-format.

[Prazek]

clang-tidy/readability/NonConstParameterCheck.cpp

95–98 ↗

(On Diff #67506)

This looks like it could be in the same if.

[danielmarjamaki]

clang-tidy/readability/NonConstParameterCheck.cpp

95–98 ↗

(On Diff #67506)

Yes it could. But would it make the code more or less readable? It wouldn't be a 1-line condition anymore then.

[alexfh]

clang-tidy/readability/NonConstParameterCheck.cpp

95–98 ↗

(On Diff #67506)

I also think that it makes sense to merge the conditions. The problem with the current code is that it is suspicious ("Why is the same action is done in two branches? Is it a bug?"). One line condition vs two lines seems secondary in this case.

103 ↗

(On Diff #67506)

This seems too strict. What about other primitive types?

docs/clang-tidy/checks/readability-non-const-parameter.rst

22 ↗

(On Diff #67506)

Please put braces on the same line with the function header. We should keep documentation consistent with LLVM style (where there's no good reason to do otherwise).

test/clang-tidy/readability-non-const-parameter.cpp

210 ↗

(On Diff #67506)

Put braces on the previous line, please. A few other instances below.

238 ↗

(On Diff #67506)

Add a CHECK-FIXES, please.

253 ↗

(On Diff #67506)

Clang-tidy can't yet analyze multiple translation units simultaneously, so we just need to keep cases that can't be correctly handled by analyzing a single translation unit at a time in mind (and also in documentation and in tests), so that they are less surprising to the user.

However, the check can and should take care of correctly handling this case in a single translation unit (as in this test, use_functionpointer2). I'm not sure it's a frequent case, so I don't insist on fixing this right away. May be fine for a follow up.

[danielmarjamaki]

clang-tidy/readability/NonConstParameterCheck.cpp

95–98 ↗

(On Diff #67506)

ok

103 ↗

(On Diff #67506)

I am not sure which type you are talking about. As far as I see we're writing warnings about bool,char,short,int,long,long long,float,double,long double,enum pointers.

I have intentionally avoided records now to start with. It should be added, but we need to be more careful when we do it.

test/clang-tidy/readability-non-const-parameter.cpp

210 ↗

(On Diff #67506)

sorry .. of course I should run clang-format on this.

[danielmarjamaki]

Fixed review comments

[danielmarjamaki]

Fixed review comments about formatting in doc

[alexfh]

LG

clang-tidy/readability/NonConstParameterCheck.cpp

104 ↗

(On Diff #68531)

You're right. I was thinking about enums, but isIntegerType takes care of them too. As the next step, we should try to add all POD types, but that's a question for another patch.

test/clang-tidy/readability-non-const-parameter.cpp

118–136 ↗

(On Diff #68531)

PointerParameterConstnessCheck is not much better, IMO. Maybe readability-use-pointer-to-const-parameter? Or just leave it like this for now.

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

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyReadabilityModule
   AvoidConstParamsInDecls.cpp
   BracesAroundStatementsCheck.cpp
   ContainerSizeEmptyCheck.cpp
   DeletedDefaultCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionSizeCheck.cpp
   IdentifierNamingCheck.cpp
   ImplicitBoolCastCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   NamedParameterCheck.cpp
   NamespaceCommentCheck.cpp
+  NonConstParameterCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantControlFlowCheck.cpp
   RedundantStringCStrCheck.cpp
   RedundantSmartptrGetCheck.cpp
   RedundantStringInitCheck.cpp
   SimplifyBooleanExprCheck.cpp
   StaticDefinitionInAnonymousNamespaceCheck.cpp
   UniqueptrDeleteReleaseCheck.cpp

clang-tools-extra/trunk/clang-tidy/readability/NonConstParameterCheck.h

+//===--- 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/trunk/clang-tidy/readability/NonConstParameterCheck.cpp

+//===--- 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/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "ContainerSizeEmptyCheck.h"
 #include "DeletedDefaultCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
 #include "NamedParameterCheck.h"
+#include "NonConstParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
 #include "RedundantStringInitCheck.h"
 #include "SimplifyBooleanExprCheck.h"
 #include "StaticDefinitionInAnonymousNamespaceCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
 
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ImplicitBoolCastCheck>(
         "readability-implicit-bool-cast");
     CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>(
         "readability-inconsistent-declaration-parameter-name");
     CheckFactories.registerCheck<StaticDefinitionInAnonymousNamespaceCheck>(
         "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>(
         "readability-redundant-smartptr-get");
     CheckFactories.registerCheck<RedundantStringCStrCheck>(
         "readability-redundant-string-cstr");
     CheckFactories.registerCheck<RedundantStringInitCheck>(
         "readability-redundant-string-init");

clang-tools-extra/trunk/docs/ReleaseNotes.rst

- New `mpi-type-mismatch
   Flags MPI function calls with a buffer type and MPI data type mismatch.
 
 - New `performance-inefficient-string-concatenation
   <http://clang.llvm.org/extra/clang-tidy/checks/performance-inefficient-string-concatenation.html>`_ check
 
   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
 -----------------------------
 
 - Emacs integration was added.
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

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

.. toctree::
    readability-container-size-empty
    readability-deleted-default
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
    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
    readability-redundant-string-init
    readability-simplify-boolean-expr
    readability-static-definition-in-anonymous-namespace
    readability-uniqueptr-delete-release

clang-tools-extra/trunk/docs/clang-tidy/checks/readability-non-const-parameter.rst

+.. 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;
+  }
+

clang-tools-extra/trunk/test/clang-tidy/readability-non-const-parameter.cpp

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