[cfe-commits] Clang change to detect and warn about unused private members

[Jordan Rose]

On Jun 4, 2012, at 1:43 , Daniel Jasper <djasper at google.com> wrote:
> Thanks for coming up with this!! I propose a different solution which utilizes the fact that friend relationships are not transitive. Thus, if a class has a friend, it only needs to check whether all methods and nested classes of the friend are defined but does not need to consider the friend's friends.

Oh, of course! Good catch.

> Could you take one last look as the implementation of IsRecordFullyDefined has changed substantially?

Thank you for being careful. :-) Only small comments this time, I think:

- Please attach the & to the variable name for references, per LLVM style conventions (same as pointers).

- You've got an isa followed by dyn_cast for CXXMethodDecl; please change to just dyn_cast (like CXXRecordDecl).

- DenseMap has an operator[], which is a little easier to read than insert+make_pair.

- This is very nitpicky, but the style guide says to put all comments on their own line, and to end them with periods (full stops). So "// This is a template friend, give up" should be reformatted as such.

Also, it took me a while to convince myself that CXXRecordDecls coming out of getFriendDecl() are also problematic, but what convinced me is that if they don't have a TypeSourceInfo, we can't possibly see their definition. It does seem like it is possible to have CXXRecordDecls come out of getFriendDecl(), though, and that might be worth commenting.

I'm tempted to say that passing around a pair of maps to two different methods is worth a private class, but I guess it's fine the way it is.

I think that's it. Good work!

Jordan