[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:156
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+  AST = tooling::buildASTFromCode(
----------------
shuaiwang wrote:
> JonasToth wrote:
> > shuaiwang wrote:
> > > JonasToth wrote:
> > > > JonasToth wrote:
> > > > > I feel that there a multiple tests missing:
> > > > > 
> > > > > - multiple levels of pointers `int ***`, `int * const *`
> > > > > - pointers to references `int &*`
> > > > > - references to pointers `int *&`
> > > > > - ensure that having a const pointer does no influence the pointee analysis `int * const p = &i; *p = 42;`
> > > > > - a class with `operator*` + `operator->` const/non-const and the analysis for pointers to that class
> > > > > - pointer returned from a function
> > > > > - non-const reference returned 
> > > > > ```
> > > > > int& foo(int *p) {
> > > > >   return *p;
> > > > > }
> > > > > ```
> > > > > 
> > > > for the multi-level pointer mutation: it would be enough to test, that the second layer is analyzed properly, and that the `int * >const< *` would be detected.
> > > Added except for:
> > > - Anything that requires following a dereference, we need `findPointeeDerefMutation` for that.
> > > - Pointer to a class with `operator*` + `operator->`, I think those two operators doesn't matter, there's no way to accidentally invoke them from a pointer.
> > > 
> > But we want to analyze smart pointers in the future as well, not? It would be good to already prepare that in the testing department.
> > Or would the nature of `operator*` already make that happen magically?
> Yes we'll handle smart pointers, and we'll handle that in `findPointeeDerefMutation`, basically it'll look like:
> ```
> if (native pointer && derefed with *) findMutation(deref expr)
> if (smart pointer && called operator*) findMutation(operator call expr)
> if (smart pointer && called operator->) findPointeeMutation(operator call expr)
> ```
> I think it would be more clear if we can match the implementation progress with unit test cases as that shows what kind of cases starts to be supported by the change.
Ok.


================
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:61
+                      ASTUnit *AST) {
+  const auto *const S = selectFirst<Stmt>("stmt", Results);
+  const auto *const E = selectFirst<Expr>("expr", Results);
----------------
Is there a reason why the pointer is marked `const`? Usually this is not done in the codebase and I feel consistency is key (similar to `const` for values).


================
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:90
+  ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
+  const Stmt *By = Analyzer.findPointeeMutation(E);
+  std::string buffer;
----------------
You can put this line 2 lines below, 'initialize late'


================
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:639
   EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_EQ(pointeeMutatedBy(Results, AST.get()), "return x;");
 
----------------
a nice test to add would be `const int* f() { int* x; return x; }.


Repository:
  rC Clang

https://reviews.llvm.org/D52219