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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
