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