I have a few comments in addition to Jordy's:
* Since pure virtual functions can be defined, the diagnostic as it
stands might have false positives. In general, I think that's
unavoidable and acceptable, but in the specific case of a pure virtual
destructor, I think we should require that the TU contains a
definition: such a destructor must actually be defined for the class
to be useful as a base class, and may well be the only member of the
class which has an out-of-line definition, and might reasonably
contain the only use (other than a side-effect-free initialization) of
a private member.
* The check for isTemplateDecl() in IsRecordFullyDefined is redundant,
since CXXRecordDecls are never TemplateDecls.
* It looks like in-class initializers and (in-constructor) member
initializers are handled slightly differently (particularly for
non-class types where the initializer has side-effects). It would seem
more consistent with the language semantics if in-class initializers
were treated exactly like member initializers, and only considered if
they're actually used by a constructor. [... it also strikes me that a
warning for an unused in-class initializer could be useful :) ...]
* I would like to see some test cases for unions, and for anonymous
struct/union members, in particular for cases like this:
class S {
// ... no use of Aligner ...
private:
union {
void *Aligner;
unsigned char Data[8];
};
};
(See clang::DependentFunctionTempateSpecializationInfo for a
real-world example of this.) It looks like we won't diagnose that case
(because Aligner is technically public, despite being effectively a
private member of S). But I suspect we will diagnose this (and
probably shouldn't):
union S {
// ... no use of Aligner ...
private:
void *Aligner;
unsigned char Data[8];
};
Perhaps we should conservatively disable the diagnostic for union members?
Thanks!
Richard
On Mon, Jun 4, 2012 at 10:22 AM, Jordan Rose <[email protected]> wrote:
>
> On Jun 4, 2012, at 1:43 , Daniel Jasper <[email protected]> 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
>
> _______________________________________________
> cfe-commits mailing list
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
