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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

LGTM, with a few nits and a question. Please wait a bit if @aaron.ballman or @george.karpenkov have any comments before committing.



================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:88
+bool isPointeeMutable(const Expr *Exp, const ASTContext &Context) {
+  if (const auto *PT = Exp->getType()->getAs<PointerType>()) {
+    return !PT->getPointeeType().isConstant(Context);
----------------
you can elide the braces here


================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:101
+              return false;
+            // Check whether returning non-const reference.
+            if (const auto *RT =
----------------
Please make this a full sentence (adding `... the overloaded 'operator*' is returning ...` is enough.)


================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:227
+  // `Exp` can be *directly* mutated if the type of `Exp` is not const.
+  // Bail out early otherwise.
+  if (Exp->getType().isConstant(Context))
----------------
This comment is one sentence, just replace `const. Bail` with `const, bail`


================
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:87
+  EXPECT_TRUE(isPointeeMutated(Results, AST));
+  const auto *const S = selectFirst<Stmt>("stmt", Results);
+  const auto *const E = selectFirst<Expr>("expr", Results);
----------------
pointer is `const`, please remove the last `const`


================
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:645
+  // TODO: this should work when casts of pointers are handled.
+  // EXPECT_EQ(pointeeMutatedBy(Results, AST.get()), "return x;");
 
----------------
I am confused. The `const int*` guarantees, that `x`-pointee is not mutated. Why shall this be diagnosed as a mutation?


Repository:
  rC Clang

https://reviews.llvm.org/D52219