[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

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