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 
- *this is passed as non-const reference param to a function -> *this can't be 

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 

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?

  rCTE Clang Tools Extra

cfe-commits mailing list

Reply via email to