JonasToth 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 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 :))

  rCTE Clang Tools Extra

cfe-commits mailing list

Reply via email to