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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Looks like a good start! I think getting the pointers right will be most difficult, because of the multiple levels of indirection they allow. Do you think it would be possible to the analysis for `>const?< int ***`-cases? (recursively checking through the pointer levels)



================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:198
 const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
+  // `Exp` can be *directly* mutated if the type of `Exp` is not const.
+  // Bail out early otherwise.
----------------
Just to be sure that i understand:
the changes here are more performance optimizations then directly related to detect pointee mutations?


================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:440
+      return nullptr;
+  } else if (const auto *RT = Exp->getType()->getAs<RecordType>()) {
+    if (const CXXRecordDecl *RecordDecl = RT->getAsCXXRecordDecl()) {
----------------
no `else` after return. this makes the short circuit logic clearer


================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:442
+    if (const CXXRecordDecl *RecordDecl = RT->getAsCXXRecordDecl()) {
+      const bool ExpIsConst = Exp->getType().isConstant(Context);
+      if (llvm::any_of(
----------------
values are not marked as const by the llvm code guideline


================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:459
+  }
+  const bool ExpIsPointer = Exp->getType()->getAs<PointerType>();
+
----------------
implicit conversion from pointer to bool? Please make that better visible and please dont use const on values. 
Not sure for the matchers, I have seen both, but they should be treated as values as well i think.


================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:463
+  // A member function is assumed to be non-const when it is unresolved.
+  // We don't handle pointer-like type here as non-const member function of
+  // pointee can't be directly invoked without dereferencing the pointer-like
----------------
Please make that sentence clearer, i could not understand it (only interpret :D)


================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:481
+  const auto AsArg =
+      anyOf(callExpr(hasAnyArgument(equalsNode(Exp))),
+            cxxConstructExpr(hasAnyArgument(equalsNode(Exp))),
----------------
shouldn't be the constness of the argument considered here?


================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:488
+
+  // Returned.
+  const auto AsReturnValue = returnStmt(hasReturnValue(equalsNode(Exp)));
----------------
Either remove the comment or make it a full sentence please. I think the variable naming is clear enough to elide


================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:500
+const Stmt *ExprMutationAnalyzer::findPointeeCastMutation(const Expr *Exp) {
+  // Only handling LValueToRValue for now for easier unit testing during
+  // implementation.
----------------
Please add a todo to ensure the missing casts are not forgotten


================
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:43
   const auto *const E = selectFirst<Expr>("expr", Results);
+  assert(E);
   return ExprMutationAnalyzer(*S, AST->getASTContext()).isMutated(E);
----------------
maybe even `assert(E && S)`?


================
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:67
+    if (const auto *DRE = dyn_cast<DeclRefExpr>(E)) {
+      if (DRE->getNameInfo().getAsString()[0] == 'p')
+        Finder = PointeeMutationFinder;
----------------
That doesn't look like a super idea. It is super hidden that variable 'p*' will be analyzed for pointee mutation and others aren't. Especially in 12 months and when someone else wants to change something, this will be the source of headaches.

Don't you think there could be a better way of doing this switch?


================
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:156
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+  AST = tooling::buildASTFromCode(
----------------
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;
}
```



================
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:175
+  Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("p->mf<T>()"));
 
----------------
Could you please add the `EXPECT_FALSE(isMutated()) && EXPECT_TRUE(isPointeeMutated()`?
Maybe a short helper for that like `isOnlyPointeeMutated()` would be nice.

Here and the other cases as well


================
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:686
+  AST = tooling::buildASTFromCode(
+      "void g(const int*); void f() { const int x[2] = {}; g(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
----------------
Please add a mutating example for array access through a pointer (as its very common in C-style code).


Repository:
  rC Clang

https://reviews.llvm.org/D52219