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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> 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

Yes. That is similar behaviour to checking if a function can be `noexcept`. But this is all future stuff :)

> 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 cant follow you on that one. I consider every path that allows future modification (like taking a non-const reference/pointer) as `cant be const`. That would be enough, wouldnt it?
A separate function can only modify a variable if it has some form of non-const handle to it, which must have been created at some point.

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

You are probably right. The only zero-false positive case is: "only const methods called". One could split implementation of a class into several translation units and render the analysis approach useless.

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

Example as reference for later: https://godbolt.org/g/cvhoUN
I will add such cases to my tests.

@shuaiwang Are you in IRC from time to time? I think it will be better to discuss changes in chat, when i start to introduce your approach here.

> isModified(Expr, Stmt), that checks whether Expr is (potentially) modified within Stmt, is something I believe to be quite useful:

What i understand from your comments and code:

- implement a utility taking `DeclRefExpr` and check if there is modification in some scope (`Stmt`, e.g. `CompoundStmt`) -> `isModified` or `cantBeModifiedWithin`
- match all relevant non-const variable declarations as potential candidates
- track all uses of the candidates and check for modification in their scope

One Note: We could include variables in unnamed namespaces and `static` declared variables. They have TU scope.

My deviations:

- a variable should be considered modified if a non-const handle is create from it, even if that handle does not modify its referenced value. As first step, those handles should be marked const, then the original value can be marked as const. That is required to produce compiling code after potential code-transformation.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444