[PATCH] D52008: [analyzer] Handle forwarding reference better in ExprMutationAnalyzer.

[Jonas Toth via Phabricator via cfe-commits]

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