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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
