shuaiwang marked 2 inline comments as done. shuaiwang 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: > 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 Added these tests: * std::move copy only type doesn't mutate * std::move move only type does mutate * std::move copiable & movable type does mutate * std::move const copiable & movable type doesn't mutate Didn't test array because array can't be assigned (so copy/move isn't relevant) and passing array to function can only be done via reference (passing by value results in array to pointer decay) so no copy/move there as well. Repository: rC Clang https://reviews.llvm.org/D52120 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits