[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:21-25
+  for (const auto *Init : Node.capture_inits()) {
+    if (Init == E)
+      return true;
+  }
+  return false;
----------------
This can be replaced with `return llvm::find(Node.capture_inits(), E);`


================
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:34-38
+const ast_matchers::internal::VariadicDynCastAllOfMatcher<Stmt, CXXTypeidExpr>
+    cxxTypeidExpr;
+
+const ast_matchers::internal::VariadicDynCastAllOfMatcher<Stmt, CXXNoexceptExpr>
+    cxxNoexceptExpr;
----------------
I think these should be exposed as public AST matchers, as they're both generally useful. It can be done in a follow-up patch, or done before landing this patch, either is fine by me.


================
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:67
+
+bool ExprMutationAnalyzer::isUnevaluated(const Expr *Exp) {
+  return selectFirst<Expr>(
----------------
What about other unevaluated expressions, like `typeof`, `_Generic`, etc? You should search for `ExpressionEvaluationContext::Unevaluated` in Clang to see where we set up an unevaluated expression evaluation context to find all of the situations.


================
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:72
+                 findAll(expr(equalsNode(Exp),
+                              anyOf(hasAncestor(typeLoc()),
+                                    hasAncestor(expr(anyOf(
----------------
What is this trying to match with the `typeLoc()` matcher?


================
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:74-75
+                                    hasAncestor(expr(anyOf(
+                                        unaryExprOrTypeTraitExpr(),
+                                        cxxTypeidExpr(), cxxNoexceptExpr())))))
+                             .bind("expr")),
----------------
I think these are only approximations to testing whether it's unevaluated. For instance, won't this match these expressions, despite them being evaluated?
```
// C++
#include <typeinfo>

struct B {
  virtual ~B() = default;
};

struct D : B {};

B& get() {
  static B *b = new D;
  return *b;
}

void f() {
  (void)typeid(get()); // Evaluated, calls get()
}

/* C99 and later */
#include <stddef.h>

void f(size_t n) {
  (void)sizeof(int[++n]); // Evaluated, increments n
}
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45679