[PATCH] D45679: [clang-tidy] Add a helper function isModified, that checks whether an expression is modified within a statement.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

You are doing a great job and i learn new stuff :)

Inspired by the analysis tool in clangs repo:

- What do you think about having these functions in a class? Now, we need to recalculate and reanalyze the scope for every variable, multiple times (reference tracking). It would be nice to do it as lazy as possible and memorize the results. Especially addressing the use-case for the const-check, storing that a reference is not modified will save a lot of work = performance
- Do we need to distinguish between `Espaced` and `Modified`? Having only two states will simplify some calculations (`e.g. if (std::none_of(Expr, isModified) return EMK_Const`)
- i think the multiple analysis sections could be functions on their own. If you create a class for the check, these should be private methods or helpers. At the moment, some sections are hard to understand and some simplifications could be made.
- what do you think creating a real `ModificationReport` that is stored per `Expr`? That can be helpful for a check like `readability-complex-modification`, dependency analysis and others. The `Chain` is in that direction, but not consistent from what i see. Storing this chain in the potential `ModificationAnalyzer` class would be superb.



================
Comment at: clang-tidy/utils/ASTUtils.cpp:125
+  const auto AsNonConstRefArg =
+      anyOf(callExpr(NonConstRefParam), cxxConstructExpr(NonConstRefParam));
+
----------------
I am suprised that `callExpr` does not cover constructor calls. Or is there more?


================
Comment at: clang-tidy/utils/ASTUtils.cpp:149
+      Stm, *Context);
+  if (const auto *S = selectFirst<Stmt>("mod", ModOrEsc)) {
+    if (Chain != nullptr)
----------------
Having the history for the trivial modifications would be nice, too.

I think treating all kinds of modifications is best.


================
Comment at: clang-tidy/utils/ASTUtils.cpp:160
+
+  const auto isExprModified = [&](ArrayRef<BoundNodes> Results) {
+    for (const auto &Node : Results) {
----------------
Having such a lambda is somewhat weird and redundant, because it mimics the original function.

I think that should be refactored with a short discussion in the general comments if thats ok for you.


================
Comment at: clang-tidy/utils/ASTUtils.cpp:180
+      Stm, *Context);
+  if (const auto Kind = isExprModified(MemberExprs))
+    return Kind;
----------------
The implicit `NotModified` == 0 is hard to see.
Maybe a transition towards a `std::any_of(Expr, isModified)` is more readable. (see general comment)


================
Comment at: clang-tidy/utils/ASTUtils.cpp:188
+      Stm, *Context);
+  if (const auto Kind = isExprModified(SubscriptExprs))
+    return Kind;
----------------
Same + code duplication.


================
Comment at: clang-tidy/utils/ASTUtils.cpp:200
+      Stm, *Context);
+  if (const auto Kind = isExprModified(Casts))
+    return Kind;
----------------
dito


================
Comment at: clang-tidy/utils/ASTUtils.cpp:220
+        Stm, *Context);
+    if (const auto Kind = isExprModified(Exprs))
+      return Kind;
----------------
This section of the code is hard to understand.


================
Comment at: clang-tidy/utils/ASTUtils.cpp:222
+      return Kind;
+    if (Chain != nullptr)
+      Chain->pop_back();
----------------
The pop is not clear to me


================
Comment at: clang-tidy/utils/ASTUtils.cpp:246
+    const auto Exprs = match(
+        findAll(declRefExpr(to(equalsNode(DeclNode.getNodeAs<Decl>("decl"))))
+                    .bind("expr")),
----------------
Code duplication for finding all `declRefExpr` to that expr.


================
Comment at: clang-tidy/utils/ASTUtils.cpp:252
+    if (Chain != nullptr)
+      Chain->pop_back();
+  }
----------------
same + code duplication


================
Comment at: unittests/clang-tidy/IsModifiedTest.cpp:138
+
+TEST(IsModifiedTest, ConstOperator) {
+  const auto AST = tooling::buildASTFromCode(
----------------
Could you please add tests for overloaded operators as free functions?

Arithmetic operators can usually be free functions and take by const& or &.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45679