[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.

I like your refactoring very much and i think we have a state that is close to commit.

My clang-tidy check is already based on top of your functionality, but i can not work on it this weekend and next week is already busy for me. I decided to analysis values and references only for now, and work on pointer semantics later, because of some uncertainty how to handle different things.

Right now, nothing comes to my mind that might be missing. Maybe you could add small helpers and utilities, that take a `varDecl` and run the analysis (implemented in my check). That would move the last piece into this section :)



================
Comment at: clang-tidy/utils/ASTUtils.cpp:125
+  const auto AsNonConstRefArg =
+      anyOf(callExpr(NonConstRefParam), cxxConstructExpr(NonConstRefParam));
+
----------------
shuaiwang wrote:
> 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.
Are move and copy constructor covered by this?


================
Comment at: clang-tidy/utils/ASTUtils.cpp:149
+      Stm, *Context);
+  if (const auto *S = selectFirst<Stmt>("mod", ModOrEsc)) {
+    if (Chain != nullptr)
----------------
shuaiwang wrote:
> 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?
I think i was mostly irritated and it is irrelevant with the new semantics (which i think are good for now!).


================
Comment at: clang-tidy/utils/ASTUtils.cpp:222
+      return Kind;
+    if (Chain != nullptr)
+      Chain->pop_back();
----------------
shuaiwang wrote:
> 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.
Ok.


================
Comment at: clang-tidy/utils/ASTUtils.cpp:233
+                                   conditionalOperator(anyOf(
+                                       hasTrueExpression(equalsNode(&Exp)),
+                                       hasFalseExpression(equalsNode(&Exp)))))),
----------------
shuaiwang wrote:
> 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 */ }
> 
I was wondering about the ternaryOperator and if some suprises might arise from it. 

Thats something i will investigate next week within the clang-tidy check.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45679