JonasToth added a comment.
The new functionality looks very good. It can be used in a readability check
that suggests `const` for parameters.
================
Comment at: include/clang/Analysis/Analyses/ExprMutationAnalyzer.h:32
const Stmt *findMutation(const Expr *Exp);
+ const Stmt *findDeclMutation(const Decl *Dec);
----------------
lebedev.ri wrote:
> Thanks!
> I know this has performance implications, but those will exist even if one
> has this in his own code.
The analysis itself should not be executed twice, as it is cached. Only the way
to figuring out that its cached will be run. I think its acceptable.
================
Comment at: include/clang/Analysis/Analyses/ExprMutationAnalyzer.h:60
+public:
+ FunctionParmMutationAnalyzer(const FunctionDecl &Func, ASTContext &Context);
+
----------------
Why do we need a separate class for this?
I think you can just create another object of `ExprMutAnalyzer` and do the
analysis with `findDeclMutation(FunctioParm)`.
The `Stmt` is the `functionDecl().getBody()`. Right now it could be that you
pass in an functionDecl without body.
Could there something happen with extern templates that the body is not
accessible?
================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:201
equalsNode(Exp), parmVarDecl(hasType(nonConstReferenceType())));
+ const auto NotInstantiated = unless(hasDeclaration(isInstantiated()));
const auto AsNonConstRefArg = anyOf(
----------------
I think this will not all cases correctly.
Say
```
template <typename T>
struct Foo {
static void bar() { SomethingWith(T()); }
};
```
`bar` it self is not a template and `NotInstantiated` will be `false` (only 90%
sure on that) as the declaration of `bar` does not match.
In another check I had to do this: `unless(has(expr(anyOf(isTypeDependent(),
isValueDependent()))))` to ensure that there are no unresolved constructs in
the stmt.
================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:318
+ parmVarDecl(hasType(nonConstReferenceType())).bind("parm"));
+ const auto IsInstantiated = hasDeclaration(isInstantiated());
+ const auto FuncDecl = hasDeclaration(functionDecl().bind("func"));
----------------
Same instantiation concerns
================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:367
+ // function body, check them eagerly here since they're typically trivial.
+ for (const CXXCtorInitializer *Init : Ctor->inits()) {
+ ExprMutationAnalyzer InitAnalyzer(*Init->getInit(), Context);
----------------
Just to be sure, this will recurse ?
```
struct Foo {
std::string s;
Foo(std::string s) : s (std::move(s)) {}
};
```
`std::move` will be resolved through the new mechanism right?
================
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:625
+ Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("g(y, x)"));
+}
----------------
Please add a `Results(declRefTo("y"), notMutated(y)`, same above
================
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:659
+}
+
TEST(ExprMutationAnalyzerTest, ArrayElementModified) {
----------------
There are tests missing for the constructors. Please ensure that `std::move`
and `std::forward` are handled properly.
Repository:
rC Clang
https://reviews.llvm.org/D52008
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
