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

[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<ParmVarDecl>("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<CallExpr>("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 &ParamInfo : 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<ImplicitCastExpr>(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<struct ParInfo, 5> 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<struct ParInfo, 5> 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