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