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

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang added a comment.

In https://reviews.llvm.org/D52008#1232923, @lebedev.ri wrote:

> Thanks for working on this! I tried, and it appears to not fix the issue at hand.
>
> - ``` struct C1 { C1(const C1* c, int num); };
>
>   int x = 0; auto y = std::make_unique<C1>(nullptr, x); // <- still considered a mutation? ``` * ``` struct C3 {}; // some class
>
>   struct C2 { C2(const int* whatever, int n, C3 zz); };
>
>   int x = 0; std::vector<C2> v; v.emplace_back(nullptr, x, {}); // <- still considered a mutation? ```
>
>   And so on. These are hand-reduced, so hopefully you can reproduce?


I've patched https://reviews.llvm.org/D51870 and tried these cases, they work correctly with this change plus the change making `std::move` & `std::forward` considered casts. I also tested

  struct D {
    D(int&);
  };
  void h() {
    std::vector<D> v;
    for (int i = 0; i < 10; ++i) {
      v.emplace_back(i);
    }
  }

and that's also correctly considered as mutation at `emplace_back`



================
Comment at: include/clang/Analysis/Analyses/ExprMutationAnalyzer.h:60
+public:
+  FunctionParmMutationAnalyzer(const FunctionDecl &Func, ASTContext &Context);
+
----------------
JonasToth wrote:
> shuaiwang wrote:
> > JonasToth wrote:
> > > 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?
> > It's mostly for the special handling of constructors in which case initializer list also could mutate param.
> Hmm, still why not within `ExprMutAnalyzer`?
> You could make it a class living within ExprMutAnalyzer in the private section. Then its clear its an implementation detail.
Oh actually I'm planning to use this also in UnnecessaryValueParamCheck


================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:201
       equalsNode(Exp), parmVarDecl(hasType(nonConstReferenceType())));
+  const auto NotInstantiated = unless(hasDeclaration(isInstantiated()));
   const auto AsNonConstRefArg = anyOf(
----------------
JonasToth wrote:
> 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.
I think it's fine here since we only care about skipping those with forwarding references.


================
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);
----------------
JonasToth wrote:
> 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?
This diff along won't handle `std::move` "properly" yet.
We'll look into definition of `std::move`, it'll be something like
```
return static_cast<remove_reference<T>::type&&>(t);
```
and we'll simply conclude `t` is mutated: because it's being returned.

So for you case, we'll see `s` is mutated by `std::move(s)` and stop there, when we treat `std::move` as cast, we'll go one step further and found `std::move(s)` is passed as non-const argument to constructor of std::string, and we'll stop there concluding `s` is mutated.


Repository:
  rC Clang

https://reviews.llvm.org/D52008