[PATCH] D15332: new clang-tidy checker readability-non-const-parameter

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:44
@@ +43,3 @@
+    addParm(Parm);
+  } else if (const CXXConstructorDecl *Ctor =
+                 Result.Nodes.getNodeAs<CXXConstructorDecl>("Ctor")) {
----------------
`const auto *Ctor`

================
Comment at: docs/clang-tidy/checks/readability-non-const-parameter.rst:7
@@ +6,3 @@
+Finds function pointer parameters that should be const. When const is used
+properly, many mistakes can be avoided. Advantages when using const properly:
+ * avoid that data is unintentionally modified
----------------
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.

================
Comment at: docs/clang-tidy/checks/readability-non-const-parameter.rst:8
@@ +7,3 @@
+properly, many mistakes can be avoided. Advantages when using const properly:
+ * avoid that data is unintentionally modified
+ * get additional warnings such as using uninitialized data
----------------
"prevent unintentional modification of data"

================
Comment at: docs/clang-tidy/checks/readability-non-const-parameter.rst:10
@@ +9,3 @@
+ * get additional warnings such as using uninitialized data
+ * easier for developers to see possible side effects
+
----------------
"make it easier for developers ..."

================
Comment at: docs/clang-tidy/checks/readability-non-const-parameter.rst:12
@@ +11,3 @@
+
+clang-tidy is not strict about constness, it only warns when the constness will
+make the function interface safer.
----------------
s/clang-tidy/This check/

================
Comment at: test/clang-tidy/readability-non-const-parameter.cpp:244
@@ +243,3 @@
+  // CHECK-MESSAGES: :[[@LINE+1]]:21: warning: parameter 'p' can be const
+  void doStuff(int *p) {
+  // CHECK-FIXES: {{^}}  void doStuff(const int *p) {{{$}}
----------------
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.

================
Comment at: test/clang-tidy/readability-non-const-parameter.cpp:251
@@ +250,2 @@
+};
+
----------------
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.


http://reviews.llvm.org/D15332