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

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

2016-08-23 Thread Daniel Marjamäki via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL279507: [clang-tidy] readability-non-const-parameter: add 
new check that warns when… (authored by danielmarjamaki).

Changed prior to commit:
  https://reviews.llvm.org/D15332?vs=68531=68963#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D15332

Files:
  clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tools-extra/trunk/clang-tidy/readability/NonConstParameterCheck.h
  clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-non-const-parameter.rst
  clang-tools-extra/trunk/test/clang-tidy/readability-non-const-parameter.cpp

Index: clang-tools-extra/trunk/clang-tidy/readability/NonConstParameterCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/readability/NonConstParameterCheck.h
+++ clang-tools-extra/trunk/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 ) 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 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
Index: clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tools-extra/trunk/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 @@
 "readability-static-definition-in-anonymous-namespace");
 CheckFactories.registerCheck(
 "readability-named-parameter");
+CheckFactories.registerCheck(
+"readability-non-const-parameter");
 CheckFactories.registerCheck(
 "readability-redundant-control-flow");
 CheckFactories.registerCheck(
Index: clang-tools-extra/trunk/clang-tidy/readability/NonConstParameterCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/NonConstParameterCheck.cpp
+++ clang-tools-extra/trunk/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

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

2016-08-18 Thread Alexander Kornienko via cfe-commits

alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.

LG



Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:104
@@ +103,3 @@
+  const QualType T = Parm->getType();
+  if (!T->isPointerType() || T->getPointeeType().isConstQualified() ||
+  !(T->getPointeeType()->isIntegerType() ||

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.


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:118-136
@@ +117,21 @@
+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 : "";

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


https://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-08-18 Thread Daniel Marjamäki via cfe-commits

danielmarjamaki updated this revision to Diff 68531.
danielmarjamaki added a comment.

Fixed review comments about formatting in doc


https://reviews.llvm.org/D15332

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tidy/readability/NonConstParameterCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-non-const-parameter.rst
  test/clang-tidy/readability-non-const-parameter.cpp

Index: test/clang-tidy/readability-non-const-parameter.cpp
===
--- test/clang-tidy/readability-non-const-parameter.cpp
+++ test/clang-tidy/readability-non-const-parameter.cpp
@@ -0,0 +1,279 @@
+// 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([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(); }
+
+typedef union { void *v; } t;
+void f7(t obj);
+void callFunction7(int *p) {
+  f7((t){p});
+}
+
+void f8(int );
+void callFunction8(int *p) {
+  f8(*p);
+}
+

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

2016-08-18 Thread Daniel Marjamäki via cfe-commits

danielmarjamaki updated this revision to Diff 68528.
danielmarjamaki marked 2 inline comments as done.
danielmarjamaki added a comment.

Fixed review comments


https://reviews.llvm.org/D15332

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tidy/readability/NonConstParameterCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-non-const-parameter.rst
  test/clang-tidy/readability-non-const-parameter.cpp

Index: test/clang-tidy/readability-non-const-parameter.cpp
===
--- test/clang-tidy/readability-non-const-parameter.cpp
+++ test/clang-tidy/readability-non-const-parameter.cpp
@@ -0,0 +1,279 @@
+// 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([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(); }
+
+typedef union { void *v; } t;
+void f7(t obj);
+void callFunction7(int *p) {
+  f7((t){p});
+}
+
+void f8(int );
+void

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

2016-08-18 Thread Daniel Marjamäki via cfe-commits

danielmarjamaki marked 6 inline comments as done.


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

alexfh wrote:
> 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.
ok


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();

alexfh wrote:
> This seems too strict. What about other primitive types? 
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.



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)
+{

alexfh wrote:
> Put braces on the previous line, please. A few other instances below.
sorry .. of course I should run clang-format on this.


https://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-08-16 Thread 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-08-15 Thread Daniel Marjamäki via cfe-commits

danielmarjamaki added inline comments.


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

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.


https://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-08-12 Thread Piotr Padlewski via cfe-commits

Prazek added a subscriber: Prazek.


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

This looks like it could be in the same if.


https://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-08-10 Thread Daniel Marjamäki via cfe-commits

danielmarjamaki marked 2 inline comments as done.


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:245
@@ +244,3 @@
+  C c(p);
+}
+

I have added tests and fixed FPs in latest diff.


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:252
@@ +251,3 @@
+
+class Warn {
+public:

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?



https://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-08-10 Thread Daniel Marjamäki via cfe-commits

danielmarjamaki updated this revision to Diff 67504.
danielmarjamaki added a comment.

fixed more review comments


https://reviews.llvm.org/D15332

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tidy/readability/NonConstParameterCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-non-const-parameter.rst
  test/clang-tidy/readability-non-const-parameter.cpp

Index: test/clang-tidy/readability-non-const-parameter.cpp
===
--- test/clang-tidy/readability-non-const-parameter.cpp
+++ test/clang-tidy/readability-non-const-parameter.cpp
@@ -0,0 +1,283 @@
+// 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([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(); }
+
+typedef union { void *v; } t;
+void f7(t obj);
+void callFunction7(int *p) {
+  f7((t){p});
+}
+
+void f8(int );
+void callFunction8(int *p) {
+  f8(*p);
+}
+
+// Don't warn about nonconst function

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

2016-08-10 Thread Daniel Marjamäki via cfe-commits

danielmarjamaki marked 10 inline comments as done.


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:117-135
@@ +116,21 @@
+// 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) {{{$}}

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?


https://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-08-10 Thread Daniel Marjamäki via cfe-commits

danielmarjamaki updated this revision to Diff 67500.
danielmarjamaki added a comment.

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


https://reviews.llvm.org/D15332

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tidy/readability/NonConstParameterCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-non-const-parameter.rst
  test/clang-tidy/readability-non-const-parameter.cpp

Index: test/clang-tidy/readability-non-const-parameter.cpp
===
--- test/clang-tidy/readability-non-const-parameter.cpp
+++ test/clang-tidy/readability-non-const-parameter.cpp
@@ -0,0 +1,251 @@
+// 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([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(); }
+
+typedef union { void *v; } t;
+void f7(t obj);
+void callFunction7(int *p) {
+  f7((t){p});
+}
+
+void f8(int );
+void callFunction8(int *p) {
+

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

2016-04-01 Thread Eugene Zelenko via cfe-commits

Eugene.Zelenko added a comment.

Please also mention check in  docs/ReleaseNotes.rst.



Comment at: docs/clang-tidy/checks/readability-non-const-parameter.rst:15
@@ +14,3 @@
+
+  // warning here; the declaration "const char *p" would make the function
+  // interface safer.

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



http://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-04-01 Thread 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("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 = 

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-03-22 Thread Richard via cfe-commits

LegalizeAdulthood added inline comments.


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:116-134
@@ +115,21 @@
+
+// CHECK-MESSAGES: :[[@LINE+1]]:18: warning: parameter 'p' can be const
+int return1(int *p) {
+  // CHECK-FIXES: {{^}}int return1(const int *p) {{{$}}
+  return *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: parameter 'p' can be const
+const int *return2(int *p) {
+  // CHECK-FIXES: {{^}}const int *return2(const int *p) {{{$}}
+  return p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: parameter 'p' can be const
+const int *return3(int *p) {
+  // CHECK-FIXES: {{^}}const int *return3(const int *p) {{{$}}
+  return p + 1;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:27: warning: parameter 'p' can be const
+const char *return4(char *p) {

danielmarjamaki wrote:
> rsmith wrote:
> > 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*.
> 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?
> 
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.


http://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-03-19 Thread Richard Smith via cfe-commits

rsmith added inline comments.


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:116-134
@@ +115,21 @@
+
+// CHECK-MESSAGES: :[[@LINE+1]]:18: warning: parameter 'p' can be const
+int return1(int *p) {
+  // CHECK-FIXES: {{^}}int return1(const int *p) {{{$}}
+  return *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: parameter 'p' can be const
+const int *return2(int *p) {
+  // CHECK-FIXES: {{^}}const int *return2(const int *p) {{{$}}
+  return p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: parameter 'p' can be const
+const int *return3(int *p) {
+  // CHECK-FIXES: {{^}}const int *return3(const int *p) {{{$}}
+  return p + 1;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:27: warning: parameter 'p' can be const
+const char *return4(char *p) {

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*.


http://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-03-15 Thread Daniel Marjamäki via cfe-commits

danielmarjamaki updated this revision to Diff 50705.
danielmarjamaki marked 3 inline comments as done.
danielmarjamaki added a comment.

Fixed review comments


http://reviews.llvm.org/D15332

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tidy/readability/NonConstParameterCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-non-const-parameter.rst
  test/clang-tidy/readability-non-const-parameter.cpp

Index: test/clang-tidy/readability-non-const-parameter.cpp
===
--- test/clang-tidy/readability-non-const-parameter.cpp
+++ test/clang-tidy/readability-non-const-parameter.cpp
@@ -0,0 +1,251 @@
+// 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: parameter 'last' can be 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: parameter 'p' can be const
+void assign1(int *p) {
+  // CHECK-FIXES: {{^}}void assign1(const int *p) {{{$}}
+  const int *q;
+  q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: parameter 'p' can be const
+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: parameter 'p' can be const
+void init1(int *p) {
+  // CHECK-FIXES: {{^}}void init1(const int *p) {{{$}}
+  const int *q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:17: warning: parameter 'p' can be const
+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: parameter 'p' can be const
+int return1(int *p) {
+  // CHECK-FIXES: {{^}}int return1(const int *p) {{{$}}
+  return *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: parameter 'p' can be const
+const int *return2(int *p) {
+  // CHECK-FIXES: {{^}}const int *return2(const int *p) {{{$}}
+  return p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: parameter 'p' can be const
+const int *return3(int *p) {
+  // CHECK-FIXES: {{^}}const int *return3(const int *p) {{{$}}
+  return p + 1;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:27: warning: parameter 'p' can be const
+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([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(); }
+
+typedef union { void *v; } t;
+void f7(t obj);
+void callFunction7(int *p) {
+  f7((t){p});
+}
+
+void f8(int );
+void callFunction8(int *p) {
+  f8(*p);
+}
+
+// Don't warn about nonconst

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

2016-03-15 Thread Daniel Marjamäki via cfe-commits

danielmarjamaki marked 12 inline comments as done.


Comment at: docs/clang-tidy/checks/readability-non-const-parameter.rst:6
@@ +5,3 @@
+
+Finds function parameters that should be const. When const is used properly,
+many mistakes can be avoided. Advantages when using const properly:

hokein wrote:
> Looks like what the document says isn't consistent with the check, since the 
> check only finds non-const pointer parameter. 
I changed "Finds function parameters.." to "Finds function pointer 
parameters..".


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:3
@@ +2,3 @@
+
+// Currently the checker only warns about pointer arguments.
+//

hokein wrote:
> It makes sense to move this document to the `rst`, I think.
Done. First line in rst will now say: "Finds function pointer parameters that 
should be const".


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:219
@@ +218,3 @@
+public:
+  C(int *p) : p(p) {}
+private:

hokein wrote:
> Please add a test case:
> 
> ```
> class C {
> public:
>   C(int *p) : p(p) {}
> private:
>   const int *p;
> };
> ```
> 
> BTW, does the check support class method?
Thanks, I added such class.

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



http://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-03-14 Thread Haojian Wu via cfe-commits

hokein added inline comments.


Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:24
@@ +23,3 @@
+
+  // C++ constructor..
+  Finder->addMatcher(cxxConstructorDecl().bind("Ctor"), this);

Extra `.` at the end.


Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:124
@@ +123,3 @@
+void NonConstParameterCheck::diagnoseNonConstParameters() {
+  for (auto It : Parameters) {
+const ParmVarDecl *Par = It.first;

const auto 


Comment at: clang-tidy/readability/NonConstParameterCheck.h:53
@@ +52,3 @@
+  /// and CanNotBeConst is true the Parameter is marked as not-const.
+  /// The CanNotBeConst are updated as sub expressions are visited.
+  void markCanNotBeConst(const Expr *E, bool CanNotBeConst);

s/are/is


Comment at: docs/clang-tidy/checks/readability-non-const-parameter.rst:6
@@ +5,3 @@
+
+Finds function parameters that should be const. When const is used properly,
+many mistakes can be avoided. Advantages when using const properly:

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


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:3
@@ +2,3 @@
+
+// Currently the checker only warns about pointer arguments.
+//

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


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:15
@@ +14,3 @@
+
+// CHECK-MESSAGES: :[[@LINE+1]]:29: warning: parameter 'last' can be const 
[readability-non-const-parameter]
+void warn1(int *first, int *last) {

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. 


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:219
@@ +218,3 @@
+public:
+  C(int *p) : p(p) {}
+private:

Please add a test case:

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

BTW, does the check support class method?


http://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-03-14 Thread Daniel Marjamäki via cfe-commits

danielmarjamaki added a comment.

ping.


http://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-03-08 Thread Daniel Marjamäki via cfe-commits

danielmarjamaki updated this revision to Diff 50032.
danielmarjamaki added a comment.

Updated documentation


http://reviews.llvm.org/D15332

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tidy/readability/NonConstParameterCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-non-const-parameter.rst
  test/clang-tidy/readability-non-const-parameter.cpp

Index: test/clang-tidy/readability-non-const-parameter.cpp
===
--- test/clang-tidy/readability-non-const-parameter.cpp
+++ test/clang-tidy/readability-non-const-parameter.cpp
@@ -0,0 +1,231 @@
+// 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: parameter 'last' can be 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: parameter 'p' can be const [readability-non-const-parameter]
+void assign1(int *p) {
+  // CHECK-FIXES: {{^}}void assign1(const int *p) {{{$}}
+  const int *q;
+  q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: parameter 'p' can be const [readability-non-const-parameter]
+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: parameter 'p' can be const [readability-non-const-parameter]
+void init1(int *p) {
+  // CHECK-FIXES: {{^}}void init1(const int *p) {{{$}}
+  const int *q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:17: warning: parameter 'p' can be const [readability-non-const-parameter]
+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: parameter 'p' can be const [readability-non-const-parameter]
+int return1(int *p) {
+  // CHECK-FIXES: {{^}}int return1(const int *p) {{{$}}
+  return *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: parameter 'p' can be const [readability-non-const-parameter]
+const int *return2(int *p) {
+  // CHECK-FIXES: {{^}}const int *return2(const int *p) {{{$}}
+  return p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: parameter 'p' can be const [readability-non-const-parameter]
+const int *return3(int *p) {
+  // CHECK-FIXES: {{^}}const int *return3(const int *p) {{{$}}
+  return p + 1;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:27: warning: parameter 'p' can be const [readability-non-const-parameter]
+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([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

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

2016-03-08 Thread Daniel Marjamäki via cfe-commits

danielmarjamaki added inline comments.


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:3
@@ +2,3 @@
+
+// Currently the checker only warns about pointer arguments.
+//

LegalizeAdulthood wrote:
> danielmarjamaki wrote:
> > LegalizeAdulthood wrote:
> > > 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.
> > 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.
> 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 );
> ```
> 
> becoming
> 
> ```
> void foo(const std::string );
> ```
> 
> when `s` is treated as a read-only value within the implementation of `foo`.
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.



http://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-02-17 Thread Richard via cfe-commits

LegalizeAdulthood added inline comments.


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:3
@@ +2,3 @@
+
+// Currently the checker only warns about pointer arguments.
+//

danielmarjamaki wrote:
> LegalizeAdulthood wrote:
> > 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.
> 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.
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 );
```

becoming

```
void foo(const std::string );
```

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


http://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-02-17 Thread Aaron Ballman via cfe-commits

On Wed, Feb 17, 2016 at 2:39 AM, Daniel Marjamäki
 wrote:
> danielmarjamaki marked 2 inline comments as done.
>
> 
> Comment at: docs/clang-tidy/checks/readability-non-const-parameter.rst:15
> @@ +14,3 @@
> +
> +  // warning here; p should be const
> +  char f1(char *p) {
> 
> LegalizeAdulthood wrote:
>> 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.
>>
>>
> ok I understand, will try to improve
>
> 
> Comment at: test/clang-tidy/readability-non-const-parameter.cpp:3
> @@ +2,3 @@
> +
> +// Currently the checker only warns about pointer arguments.
> +//
> 
> LegalizeAdulthood wrote:
>> 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.
> 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 think it makes sense to pass it by value only if it is not an
expensive-to-copy type (we have an AST matcher helper for that in
TypeTraits.h), or to comply with an interface.

~Aaron

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

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

2016-02-15 Thread Richard via cfe-commits

LegalizeAdulthood added inline comments.


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:3
@@ +2,3 @@
+
+// Currently the checker only warns about pointer arguments.
+//

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.


http://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-02-15 Thread Richard via cfe-commits

LegalizeAdulthood added a subscriber: LegalizeAdulthood.


Comment at: docs/clang-tidy/checks/readability-non-const-parameter.rst:15
@@ +14,3 @@
+
+  // warning here; p should be const
+  char f1(char *p) {

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.




Comment at: docs/clang-tidy/checks/readability-non-const-parameter.rst:32
@@ +31,3 @@
+  // 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.

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.


http://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-02-15 Thread Daniel Marjamäki via cfe-commits

danielmarjamaki added a comment.

In http://reviews.llvm.org/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..

http://reviews.llvm.org/D15332

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-02-15 Thread Daniel Marjamäki via cfe-commits

danielmarjamaki updated this revision to Diff 47962.
danielmarjamaki added a comment.

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/...).


http://reviews.llvm.org/D15332

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tidy/readability/NonConstParameterCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-non-const-parameter.rst
  test/clang-tidy/readability-non-const-parameter.cpp

Index: test/clang-tidy/readability-non-const-parameter.cpp
===
--- test/clang-tidy/readability-non-const-parameter.cpp
+++ test/clang-tidy/readability-non-const-parameter.cpp
@@ -0,0 +1,231 @@
+// 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: parameter 'last' can be 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: parameter 'p' can be const [readability-non-const-parameter]
+void assign1(int *p) {
+  // CHECK-FIXES: {{^}}void assign1(const int *p) {{{$}}
+  const int *q;
+  q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: parameter 'p' can be const [readability-non-const-parameter]
+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: parameter 'p' can be const [readability-non-const-parameter]
+void init1(int *p) {
+  // CHECK-FIXES: {{^}}void init1(const int *p) {{{$}}
+  const int *q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:17: warning: parameter 'p' can be const [readability-non-const-parameter]
+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: parameter 'p' can be const [readability-non-const-parameter]
+int return1(int *p) {
+  // CHECK-FIXES: {{^}}int return1(const int *p) {{{$}}
+  return *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: parameter 'p' can be const [readability-non-const-parameter]
+const int *return2(int *p) {
+  // CHECK-FIXES: {{^}}const int *return2(const int *p) {{{$}}
+  return p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: parameter 'p' can be const [readability-non-const-parameter]
+const int *return3(int *p) {
+  // CHECK-FIXES: {{^}}const int *return3(const int *p) {{{$}}
+  return p + 1;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:27: warning: parameter 'p' can be const [readability-non-const-parameter]
+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) {
+

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

2015-12-09 Thread Daniel Marjamäki via cfe-commits

danielmarjamaki marked 12 inline comments as done.


Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:44
@@ +43,3 @@
+  if (B->isAssignmentOp())
+markCanNotBeConst(B, false);
+} else if (const auto *CE = dyn_cast(S)) {

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


Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:49
@@ +48,3 @@
+  for (const auto *Arg : CE->arguments()) {
+markCanNotBeConst(Arg->IgnoreParenCasts(), true);
+  }

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.


Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:84
@@ +83,3 @@
+}
+
+void NonConstParameterCheck::onEndOfTranslationUnit() {

I believe I fixed all these.


http://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2015-12-09 Thread Daniel Marjamäki via cfe-commits

danielmarjamaki updated this revision to Diff 42292.
danielmarjamaki added a comment.

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.


http://reviews.llvm.org/D15332

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tidy/readability/NonConstParameterCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-non-const-parameter.rst
  test/clang-tidy/readability-non-const-parameter.cpp

Index: test/clang-tidy/readability-non-const-parameter.cpp
===
--- test/clang-tidy/readability-non-const-parameter.cpp
+++ test/clang-tidy/readability-non-const-parameter.cpp
@@ -0,0 +1,211 @@
+// 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: parameter 'last' can be 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: parameter 'p' can be const [readability-non-const-parameter]
+void assign1(int *p) {
+  // CHECK-FIXES: {{^}}void assign1(const int *p) {{{$}}
+  const int *q;
+  q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: parameter 'p' can be const [readability-non-const-parameter]
+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: parameter 'p' can be const [readability-non-const-parameter]
+void init1(int *p) {
+  // CHECK-FIXES: {{^}}void init1(const int *p) {{{$}}
+  const int *q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:17: warning: parameter 'p' can be const [readability-non-const-parameter]
+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: parameter 'p' can be const [readability-non-const-parameter]
+int return1(int *p) {
+  // CHECK-FIXES: {{^}}int return1(const int *p) {{{$}}
+  return *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: parameter 'p' can be const [readability-non-const-parameter]
+const int *return2(int *p) {
+  // CHECK-FIXES: {{^}}const int *return2(const int *p) {{{$}}
+  return p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: parameter 'p' can be const [readability-non-const-parameter]
+const int *return3(int *p) {
+  // CHECK-FIXES: {{^}}const int *return3(const int *p) {{{$}}
+  return p + 1;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:27: warning: parameter 'p' can be const [readability-non-const-parameter]
+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);
+}
+
+int dontwarn2(_Atomic(int) * p) {
+  return *p;
+}
+
+void callFunction1(char *p) {
+  strcpy1(p, "abc");
+}
+
+void callFunction2(char *p) {
+  strcpy1([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);
+

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

2015-12-09 Thread Alexander Kornienko via cfe-commits

alexfh added a comment.

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.


http://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2015-12-09 Thread Daniel Marjamäki via cfe-commits

danielmarjamaki marked 3 inline comments as done.
danielmarjamaki added a comment.

In http://reviews.llvm.org/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.

http://reviews.llvm.org/D15332

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2015-12-09 Thread Eugene Zelenko via cfe-commits

Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko added a comment.

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


http://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2015-12-09 Thread Daniel Marjamäki via cfe-commits

danielmarjamaki added a comment.

In http://reviews.llvm.org/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.


http://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2015-12-08 Thread Daniel Marjamäki via cfe-commits

danielmarjamaki created this revision.
danielmarjamaki added reviewers: alexfh, aaron.ballman, rsmith.
danielmarjamaki added subscribers: cfe-commits, sberg.

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.

http://reviews.llvm.org/D15332

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tidy/readability/NonConstParameterCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-non-const-parameter.rst
  test/clang-tidy/readability-non-const-parameter.c

Index: test/clang-tidy/readability-non-const-parameter.c
===
--- test/clang-tidy/readability-non-const-parameter.c
+++ test/clang-tidy/readability-non-const-parameter.c
@@ -0,0 +1,215 @@
+// 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: parameter 'last' can be 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: parameter 'p' can be const [readability-non-const-parameter]
+void assign1(int *p) {
+// CHECK-FIXES: {{^}}void assign1(const int *p) {{{$}}
+  const int *q;
+  q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: parameter 'p' can be const [readability-non-const-parameter]
+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: parameter 'p' can be const [readability-non-const-parameter]
+void init1(int *p) {
+// CHECK-FIXES: {{^}}void init1(const int *p) {{{$}}
+  const int *q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:17: warning: parameter 'p' can be const [readability-non-const-parameter]
+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: parameter 'p' can be const [readability-non-const-parameter]
+int return1(int *p) {
+// CHECK-FIXES: {{^}}int return1(const int *p) {{{$}}
+  return *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: parameter 'p' can be const [readability-non-const-parameter]
+const int *return2(int *p) {
+// CHECK-FIXES: {{^}}const int *return2(const int *p) {{{$}}
+  return p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: parameter 'p' can be const [readability-non-const-parameter]
+const int *return3(int *p) {
+// CHECK-FIXES: {{^}}const int *return3(const int *p) {{{$}}
+  return p + 1;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:27: warning: parameter 'p' can be const [readability-non-const-parameter]
+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);
+}
+
+int dontwarn2(_Atomic(int) *p) {
+  return *p;
+}
+
+
+
+void callFunction1(char *p) {
+  strcpy1(p, "abc");
+}
+
+void callFunction2(char *p) {
+

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

2015-12-08 Thread Alexander Kornienko via cfe-commits

alexfh added inline comments.


Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:20
@@ +19,3 @@
+void NonConstParameterCheck::registerMatchers(MatchFinder *Finder) {
+  // TODO: This checker doesn't handle C++ to start with. There are problems
+  // for example with parameters to template functions.

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).


Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:83
@@ +82,3 @@
+void NonConstParameterCheck::ref(const DeclRefExpr *Ref) {
+  struct ParInfo *PI = getParInfo(Ref->getDecl());
+  if (PI)

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


Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:89
@@ +88,3 @@
+struct ParInfo *NonConstParameterCheck::getParInfo(const Decl *D) {
+  for (struct ParInfo  : Params) {
+if (ParamInfo.Par == D)

aaron.ballman wrote:
> This loop can be replaced with std::find_if().
While I agree with similar comments in general, it seems that here it wouldn't 
make the code any better.


Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:168
@@ +167,3 @@
+  if (auto *Par = dyn_cast(D->getDecl())) {
+struct ParInfo *PI = getParInfo(Par);
+if (PI)

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


Comment at: docs/clang-tidy/checks/readability-non-const-parameter.rst:14
@@ +13,3 @@
+  // warning here; p should be const
+  char f1(char *p)
+  {

Let's format the code examples in LLVM style.


Comment at: test/clang-tidy/readability-non-const-parameter.c:211
@@ +210,3 @@
+// avoid fp for const pointer array
+void constPointerArray(const char *remapped[][2])
+{

clang-format?


http://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2015-12-08 Thread Aaron Ballman via cfe-commits

aaron.ballman added inline comments.


Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:43
@@ +42,3 @@
+  Finder->addMatcher(returnStmt().bind("return"), this);
+}
+

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.


Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:48
@@ +47,3 @@
+diagnoseNonConstParameters();
+Params.clear();
+  } else if (auto *Par = Result.Nodes.getNodeAs("par")) {

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;
}
```


Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:63
@@ +62,3 @@
+  } else if (auto *CE = Result.Nodes.getNodeAs("call")) {
+// TODO: Handle function calls better
+for (auto *Arg : CE->arguments()) {

Better how?


Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:89
@@ +88,3 @@
+struct ParInfo *NonConstParameterCheck::getParInfo(const Decl *D) {
+  for (struct ParInfo  : Params) {
+if (ParamInfo.Par == D)

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


Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:124
@@ +123,3 @@
+void NonConstParameterCheck::markCanNotBeConst(const Expr *E, bool Addr) {
+  if (auto *Cast = dyn_cast(E)) {
+// If expression is const then ignore usage.

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


Comment at: clang-tidy/readability/NonConstParameterCheck.h:19
@@ +18,3 @@
+
+struct ParInfo {
+  /// Function parameter

I think this struct can be local to NonConstParameterCheck.


Comment at: clang-tidy/readability/NonConstParameterCheck.h:43
@@ +42,3 @@
+private:
+  SmallVector Params;
+  void addPar(const ParmVarDecl *Par);

No need for the struct keyword, here and elsewhere.


Comment at: clang-tidy/readability/NonConstParameterCheck.h:44
@@ +43,3 @@
+  SmallVector Params;
+  void addPar(const ParmVarDecl *Par);
+  void ref(const DeclRefExpr *Ref);

addParm instead of addPar, for clarity?


Comment at: clang-tidy/readability/NonConstParameterCheck.h:45
@@ +44,3 @@
+  void addPar(const ParmVarDecl *Par);
+  void ref(const DeclRefExpr *Ref);
+  void markCanNotBeConst(const Expr *E, bool Addr);

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.


http://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

37 matches

Mail list logo