shuaiwang added a comment.
Coming from https://reviews.llvm.org/D45679, which I should have sent out much
earlier to get in front of you :p
Kidding aside this check is more mature than mine and I'm happy to help
incorporate mine into this one.
I do have a strong opinion that requires non-trivial refactoring of your diff
though: having a standalone reusable helper function like isModified(Expr,
Stmt), that checks whether Expr is (potentially) modified within Stmt, is
something I believe to be quite useful:
- performance/{ForRangeCopyCheck, UnnecessaryCopyInitialization} are using a
much less sophisticated isOnlyUsedAsConst(), and can benefit from a more
powerful one.
- I would imagine things could get messier if this check expands to also check
for turning member functions const: it's basically checking CxxThisExpr, being
a handle, is not modified within a member function, but note there's no VarDecl
for "this".
- It's cleaner to follow (non-const) reference chains and only mark a decl as
"can't be const" if everything on the chain is not modified, avoiding false
negatives when a variable is bound to a never-modified non-const reference.
- It's also cleaner to follow member access chains, e.g. "v.a.b.c = 10"
modifies "v". This one is particularly tricky because the modifying behavior
happens at "c = 10" while the variable we'd like to mark as "can't be const" is
arbitrary layers away, and MemberExpr alone doesn't warrant marking "can't be
const" because that'll introduce way too many false negatives, ... and I
haven't figure out a way to write a matcher to match
memberExpr(hasObjectExpression(memberExpr(hasObjectExpression(... <arbitrary
layers deep> ... (VarDeclRef) ...), not to mention any layer could either
member access or array subscript access.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45444
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
