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

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang added inline comments.


================
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;
----------------
aaron.ballman wrote:
> 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.
Will leave as follow up patch.


================
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:67
+
+bool ExprMutationAnalyzer::isUnevaluated(const Expr *Exp) {
+  return selectFirst<Expr>(
----------------
aaron.ballman wrote:
> 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.
I checked around and I think these are all we care about:
- decltype/typeof
- sizeof/alignof
- noexcept
- _Generic
- typeid

I've added TODO for _Generic and typeid for now because I think those need a bit more work to implement, while at the same time not supporting them for now only creates false positives from isMutated which is what we prefer over false negatives.


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


================
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:74-75
+                                    hasAncestor(expr(anyOf(
+                                        unaryExprOrTypeTraitExpr(),
+                                        cxxTypeidExpr(), cxxNoexceptExpr())))))
+                             .bind("expr")),
----------------
aaron.ballman wrote:
> 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
> }
> ```
Fixed handling of sizeof(VLA)
Added TODO for typeid


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45679