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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
