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

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang added a comment.

>>   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.

You'll also need to check:

- a member function calling another non-const member function -> *this can't be const
- *this is passed as non-const reference param to a function -> *this can't be const

Also when marking decls as "can't be const" we'll have to do record separately for modifying behaviors coming from different functions.
Which of course are all possible but code will get too complex than necessary IMO.

I think member variables are a separate topic: I think we should just treat them as globals and not check on them, the same argument that they can be accessed from multiple translation unit applies to global/namespace scoped variables and class scoped variables. We can only reliably check function/block scope variables.
(reliable meaning being able to achieve zero false positives with useful level of recall, false negatives are inevitable because whenever a modifiable reference/handle escape the current block/translation unit we'll have to assume it's modified.)

>>   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?

Yes my approach is doing multi-pass matching by calling isModified() recursively. I consider the multi-pass matching "necessary evil" because otherwise we'll have too many false negatives.
I thought about hasDescendent (hasAncestor actually) but I don't think that makes things easier: varDeclRef(hasAncestor(modifying)) would match both "v.a.b.c. = 10" and "map[v] = 10" and we'll still need to double check the modifying behavior does link back to the particular varDeclRef.

Reviewers, what are your thoughts about the approaches?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444