[PATCH] D50883: [clang-tidy] Handle unique owning smart pointers in ExprMutationAnalyzer

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang added inline comments.


================
Comment at: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp:658
+                     "void f() { UniquePtr<const S> x; x->mf(); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
----------------
JonasToth wrote:
> shuaiwang wrote:
> > JonasToth wrote:
> > > 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>>():
> > > }
> > > ```
> > Added a case with template. However I don't think we can do much whenever template appears: only the AST of **uninstantiated** template worth analyzing, because whatever analyze result (diag or fixit) would have to be applied to the template itself not the instantiations.
> > So the fact that the template argument of `f3` or `f4` could happen to be `UniquePtr` doesn't really matter when we analyze `f3` and `f4`, we have to analyze the way assuming the "worst" anyway.
> Yes, whenever there is something type-dependent we have to bail, thats why i would like to see the testcases to show that we actually bail, even when one instantiation would not modify.
> 
> I believe we do analyze all versions of the templated functions (uninstantiated and every instantiation), don't we? With that there can be conflicting results.
I see, it's the conflicting results you're going after :)
Good news is that we actually don't analyze all versions, we only analyze the version (instantiated or not) corresponding to the "scope" stmt passed into the constructor. Semantic-wise I feel this makes sense because if we're given an instantiated version we shouldn't bail out because nothing is type-dependent anymore in the instantiated version.
Also I think conflicts won't happen much in practice, most (all?) checks naturally pass in the uninstantiated version, in order to pass in an instantiated version a check needs to:
- Find an instantiation point
- Match and extract the function decl from the callExpr
- Extract function body compontStmt from the function decl
at that point the check owner likely knows pretty well what they're doing and shouldn't be surprised that analyze results conflicts if they happen to also analyze an uninstantiated version.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50883