Differential D50883
[clang-tidy] Handle unique owning smart pointers in ExprMutationAnalyzer Authored by shuaiwang on Aug 16 2018, 9:41 PM.
Details
For smart pointers like std::unique_ptr which uniquely owns the This gives better behavior for cases like this: void f(std::vector<std::unique_ptr<Foo>> v) { // undesirable analyze result of `v` as not mutated.
for (auto& p : v) {
p->mutate(); // only const member function `operator->` is invoked on `p`
}
}
Diff Detail
Event TimelineComment Actions I am suprised that this does not automatically follow from the general rules. At the end, smartpointers cant do anything else then 'normal' classes. The operator+/-> were not handled before? The mutation of SmartPtr x; x->mf(); should already be catched, not? Comment Actions Different from std::vector::operator[] which has two overloads for const and non-const access, std::unique_ptr only has one const version of operator->. Comment Actions I see, thank you :) Am 17.08.2018 um 18:55 schrieb Shuai Wang via Phabricator:
Comment Actions
I think the operator-> is transitively called, isn't it? (see andrei alexandrescu talk here: https://youtu.be/ozOgzlxIsdg?t=15m55s where he gives a nice class for generic locking) Comment Actions Right, for x->mf() (where x is some kind of smart pointer), operator-> is invoked on the smart pointer itself, and then that yields a (real) pointer, in which case the operator-> magically reappears and is applied on the real pointer. struct Foo {
void mf();
void cmf() const;
};
std::unique_ptr<Foo> p;
p->mf(); // mutates Foo
p->cmf(); // doesn't mutate Foo, but is treated as mutation for now. Can be improved when we're able to do findPointeeMutation( <pointer returned by operator->() call on p> )Comment Actions Ofc the current limitation with assuming always modification stays with my proposed tests. But these are the tests we need once implemented full analysis of pointers.
Comment Actions rebase & add test case
Comment Actions Your point is valid, that the decision of what to analyze should be done Maybe we just need some experience with real world code. The Am 11.09.2018 um 18:15 schrieb Shuai Wang via Phabricator:
Comment Actions Yeah let's see what happens in the wild and decide whether we need further actions. In any case I think that deserves a separate diff. | ||||||||||||||||||||||||||||||
Template testcases i miss:
// modifying template <typename T> void f() { UnqiuePtr<T> x; x->mf(); } // constant template <typename T> void f2() { UnqiuePtr<T> x; x->cmf(); } // indecidable for the template itself, but only the instantiations template <typename T> void f3() { T x; x->cmf(); } struct const_class { void cmf() const; } struct modifying_class { void cmf(); }; void call_template() { // don't trigger f3<UniquePtr<const_class>>(); // trigger modification f3<random_class*>(); } // again not decidable by the template itself template <typename T> void f4() { T t; *t; } struct very_weird { int& operator*() { return *new int(42); } }; void call_template_deref() { // no modification f4<int*>(); // modification, because deref is not const f4<UniquePtr<very_weird>>(): }