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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits