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

[Alexander Kornienko via cfe-commits]

alexfh requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:95-98
@@ +94,6 @@
+    const QualType T = VD->getType();
+    if (T->isPointerType() && !T->getPointeeType().isConstQualified())
+      markCanNotBeConst(VD->getInit(), true);
+    else if (T->isArrayType())
+      markCanNotBeConst(VD->getInit(), true);
+  }
----------------
danielmarjamaki wrote:
> Prazek wrote:
> > This looks like it could be in the same if.
> Yes it could. But would it make the code more or less readable? It wouldn't be a 1-line condition anymore then.
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.

================
Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:103
@@ +102,3 @@
+void NonConstParameterCheck::addParm(const ParmVarDecl *Parm) {
+  // Only add nonconst integer/float pointer parameters.
+  const QualType T = Parm->getType();
----------------
This seems too strict. What about other primitive types? 

================
Comment at: docs/clang-tidy/checks/readability-non-const-parameter.rst:22
@@ +21,3 @@
+  // interface safer.
+  char f1(char *p)
+  {
----------------
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).

================
Comment at: test/clang-tidy/readability-non-const-parameter.cpp:210
@@ +209,3 @@
+// CHECK-MESSAGES: :[[@LINE+1]]:27: warning: pointer parameter 'p' can be
+int functionpointer2(int *p)
+{
----------------
Put braces on the previous line, please. A few other instances below.

================
Comment at: test/clang-tidy/readability-non-const-parameter.cpp:238
@@ +237,3 @@
+  // CHECK-MESSAGES: :[[@LINE+1]]:11: warning: pointer parameter 'p' can be
+  C2(int *p) : p(p) {}
+private:
----------------
Add a CHECK-FIXES, please.

================
Comment at: test/clang-tidy/readability-non-const-parameter.cpp:253
@@ +252,3 @@
+class Warn {
+public:
+  // CHECK-MESSAGES: :[[@LINE+1]]:21: warning: pointer parameter 'p' can be
----------------
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.


https://reviews.llvm.org/D15332