[PATCH] D52120: [analyzer] Treat std::{move, forward} as casts in ExprMutationAnalyzer.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:387
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("std::move(x)"));
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
----------------
JonasToth wrote:
> I am thinking about the correctness of the change.
> 
> If we have this case:
> ```
> std::string s1 = "Hello World!";
> std::string s2 = std::move(s1);
> ```
> `s1` gets mutated, but it looks like it would not be considered as mutation?
> 
> On the other hand
> ```
> int i1 = 42;
> int i2 = std::move(i1);
> ```
> should resolve in a copy `i1` and therefor not be a mutation.
> 
> Could you please add tests demonstrating this difference and the correct diagnostic detection for that.
> 
> Potentially interesting:
> - Builtin types should be Copy-Only?, builtin arrays as expensive to move types for the completeness please as well
> - Types with deleted move operations, should resolve in copy too
> - Move-Only types like unique_ptr
> - Types with both move and copy constructor (vector-like)
> - and something with everything defaulted
> 
> These thoughts are just thinking and writing, I just wondered that moving a variable with std::move is not considered as mutation here, even though there are cases were it is actually a mutation.
Related: https://clang.llvm.org/extra/clang-tidy/checks/performance-move-const-arg.html


Repository:
  rC Clang

https://reviews.llvm.org/D52120