JonasToth 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 would love to, yours is more elegant :)
> 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:
Yes please!
> performance/{ForRangeCopyCheck, UnnecessaryCopyInitialization} are using a
> much less sophisticated isOnlyUsedAsConst(), and can benefit from a more
> powerful one.
I did not think about such possibilities, but maybe other analysis task could
benefit too? Warnings?
> 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".
Using my approach, you can check if any member variable is used as non-const.
Then mark this method as const, if it is not virtual.
Similar for member variables: private non-consts can be converted into consts.
> 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.
For warning only yes. But i want automatic code transformation which will break
such code if there are non-const references taken. Having it in one pass is
certainly nice. With my approach multiple runs are required if the handles are
marked as const.
> 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.
Does your approach work with the nesting? Maybe something recursive or
`hasDescendent(modifying)` could do it?
Is read your comment on this check later, i first saw your new check. Maybe we
should discuss only under one of both :) (which one is not important for me :))
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
