[PATCH] D88088: WIP [clang] improve accuracy of ExprMutAnalyzer

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Analysis/ExprMutationAnalyzer.cpp:44
+              InnerMatcher) {
+  // Unless the value is a derived class and is assigned to a
+  // reference to the base class. Other implicit casts should not
----------------
Unless the value -> Matches unless the value


================
Comment at: clang/lib/Analysis/ExprMutationAnalyzer.cpp:65
+// not have the 'arguments()' method.
+AST_MATCHER_P(InitListExpr, hasAnyArgumentExpr,
+              ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
----------------
I think I'd prefer this to be named `hasAnyInit()` to complement `hasInit()` -- these aren't really arguments.


================
Comment at: clang/lib/Analysis/ExprMutationAnalyzer.cpp:262
+          hasArgument(0, ignoringImpCasts(canResolveToExpr(equalsNode(Exp))))),
+      // operator call expression might be unresolved as well. If that is
+      // the case and the operator is called on the 'Exp' itself, this is
----------------
operator call expression -> The call operator expression


================
Comment at: clang/lib/Analysis/ExprMutationAnalyzer.cpp:264
+      // the case and the operator is called on the 'Exp' itself, this is
+      // considered a moditication.
+      cxxOperatorCallExpr(
----------------
moditication -> modification


================
Comment at: clang/lib/Analysis/ExprMutationAnalyzer.cpp:326
+      // If the initializer is for a reference type, there is no cast for
+      // the variable. Values are casted to RValue first.
+      initListExpr(
----------------
casted -> cast


================
Comment at: clang/lib/Analysis/ExprMutationAnalyzer.cpp:449
+
+  // It is possible, that containers do not provide a const-overload for their
+  // iterator accessors. If this is the case, the variable is used non-const
----------------
It is possible, that -> It is possible that


================
Comment at: clang/lib/Analysis/ExprMutationAnalyzer.cpp:452
+  // no matter what happens in the loop. This requires special detection as it
+  // is faster to find then all mutations of the loop variable.
+  // It aims at a different modification as well.
----------------
is faster to find then all -> is then faster to find all


================
Comment at: clang/lib/Analysis/ExprMutationAnalyzer.cpp:455
+  const auto HasAnyNonConstIterator =
+      anyOf(allOf(hasMethod(allOf(hasName("begin"), unless(isConst()))),
+                  unless(hasMethod(allOf(hasName("begin"), isConst())))),
----------------
Do we want to look for methods that end with `_?[Bb]egin` or `_?[Ee]nd` so that this would catch patterns like `foo_begin()`/`foo_end()`, `FooBegin()`/`FooEnd()`, or `Foo_Begin()`/`Foo_End()`?


================
Comment at: clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp:65
+
+  std::string buffer;
   for (const auto *E = selectFirst<Expr>("expr", Results); E != nullptr;) {
----------------
Was there a reason you hoisted this out of the `for` loop?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88088/new/

https://reviews.llvm.org/D88088