shuaiwang added a comment.

Coming from, 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.

  rCTE Clang Tools Extra

cfe-commits mailing list

Reply via email to