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

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang added a comment.

Regarding full dependency analysis, I think it's out of the scope of this change at least for now. We're only trying to find //one// possible point where the given `Expr` is (assumed to be) mutated. This is known to be useful at this point for use cases like "check whether const can be added" or "check whether a by-value copy can be replace with a const-by-ref capture". The analysis is also designed to be performed within a limited scope because we're mostly targeting code-style issues (for example, even if we can do the analysis across multiple functions within the same TU, it might still not be a good idea because at that point it's much less obvious to human readers.) Figuring out the full dependency will be more costly than what we're doing right now and would be overkill for the applications we'd like to apply to.

For `isOnlyUsedAsConst`, I'm intending to replace that with this (ref one of my oldest comment.)



================
Comment at: clang-tidy/utils/ASTUtils.cpp:125
+  const auto AsNonConstRefArg =
+      anyOf(callExpr(NonConstRefParam), cxxConstructExpr(NonConstRefParam));
+
----------------
JonasToth wrote:
> I am suprised that `callExpr` does not cover constructor calls. Or is there more?
Added test cases for this.
cxxConstructExpr is the only one I can think of that happens to not be covered by callExpr.


================
Comment at: clang-tidy/utils/ASTUtils.cpp:149
+      Stm, *Context);
+  if (const auto *S = selectFirst<Stmt>("mod", ModOrEsc)) {
+    if (Chain != nullptr)
----------------
JonasToth wrote:
> Having the history for the trivial modifications would be nice, too.
> 
> I think treating all kinds of modifications is best.
Could you elaborate what do you mean here?


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

In case you wonder, I was intended to put a bit more information into Chain and here we're basically trying each element of LoopVars and see whether it's modified or not, because each recursive call appends to Chain we need to push the LoopVarStmt first before doing the recursive call, but if the recursive call concluded that the var is not modified, we need to pop the LoopVarStmt back out.
Anyway this is not removed.


================
Comment at: clang-tidy/utils/ASTUtils.cpp:226
+
+  // If 'Exp' is bound to a non-const reference, check all declRefExpr to that.
+  const auto Refs = match(
----------------
JonasToth wrote:
> What about transitive references and pointers?
> It seems that only references are checked, but if a pointer is taken through that reference and vice versa, is that tracked correctly?
Yes.

As soon as an address is taken we assume it's modified. Currently we don't follow pointers.
Examples:
int a = 10;
&a; <-- taking address, assumed modification point

int b = 10;
int& c = b; <-- follow the reference
&c; <-- taking address, assumed modification point, propagates back to "b"


================
Comment at: clang-tidy/utils/ASTUtils.cpp:233
+                                   conditionalOperator(anyOf(
+                                       hasTrueExpression(equalsNode(&Exp)),
+                                       hasFalseExpression(equalsNode(&Exp)))))),
----------------
JonasToth wrote:
> If the `Exp` is an array, that will not match array subscript?
> 
> Are there other similar cases?
If `Exp` is an array, we should first follow the array subscript expression and do a recursive call.

i.e.:
int x[2];
int& y = x[0];
y = 10;
isModified(x) -> isModified(x[0]) -> isModified(y) { return true /* because y = 10 */ }



================
Comment at: clang-tidy/utils/ASTUtils.h:31
+
+enum ExprModificationKind {
+  EMK_NotModified, /// The Expr is neither modified nor escaped.
----------------
JonasToth wrote:
> Maybe you could add an `Unknown` kind, e.g. if the expression is not found or similar.
Reverted back to just bool.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45679