<br><br><div class="gmail_quote">On Mon, Jun 4, 2012 at 8:33 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I have a few comments in addition to Jordy's:<br>
<br>
* Since pure virtual functions can be defined, the diagnostic as it<br>
stands might have false positives. In general, I think that's<br>
unavoidable and acceptable, but in the specific case of a pure virtual<br>
destructor, I think we should require that the TU contains a<br>
definition: such a destructor must actually be defined for the class<br>
to be useful as a base class, and may well be the only member of the<br>
class which has an out-of-line definition, and might reasonably<br>
contain the only use (other than a side-effect-free initialization) of<br>
a private member.<br></blockquote><div><br></div><div>Do you mean that we should require that as part of this warning (i.e. mark those classes as incomplete) or do you mean we should create a separate warning for that? Also, do you think such a destructor might actually have side-effects based on the otherwise unused fields. I somewhat doubt that as we are only looking at pointers or primitive fields.. We might have something like an owning pointer, but there the destructor will never be the only "use".</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* The check for isTemplateDecl() in IsRecordFullyDefined is redundant,<br>
since CXXRecordDecls are never TemplateDecls.</blockquote><div><br></div><div> Removed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* It looks like in-class initializers and (in-constructor) member<br>
initializers are handled slightly differently (particularly for<br>
non-class types where the initializer has side-effects). It would seem<br>
more consistent with the language semantics if in-class initializers<br>
were treated exactly like member initializers, and only considered if<br>
they're actually used by a constructor. [... it also strikes me that a<br>
warning for an unused in-class initializer could be useful :) ...]<br></blockquote><div><br></div><div>Just to clarify: An in-class initializer is only used, if at least one constructor does not have a corresponding member initializer?</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* I would like to see some test cases for unions, and for anonymous<br>
struct/union members, in particular for cases like this:<br>
<br>
class S {<br>
// ... no use of Aligner ...<br>
private:<br>
union {<br>
void *Aligner;<br>
unsigned char Data[8];<br>
};<br>
};<br></blockquote><div><br></div><div>I'd like to get the first version in. Thus I'd like to add this test case together with a FIXME. The correct solution for this (and especially for anonymous structs) would be to look at the corresponding DeclContext, right?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
(See clang::DependentFunctionTempateSpecializationInfo for a<br>
real-world example of this.) It looks like we won't diagnose that case<br>
(because Aligner is technically public, despite being effectively a<br>
private member of S). But I suspect we will diagnose this (and<br>
probably shouldn't):<br>
<br>
union S {<br>
// ... no use of Aligner ...<br>
private:<br>
void *Aligner;<br>
unsigned char Data[8];<br>
};<br>
<br>
Perhaps we should conservatively disable the diagnostic for union members?<br></blockquote><div><br></div><div>Why should we not warn in this case? I think in general we might want to exempt char-arrays as they are commonly use for alignment, but other than that?</div>
<div><br></div><div>Daniel</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks!<br>
<span><font color="#888888">Richard<br>
</font></span><div><div><br>
On Mon, Jun 4, 2012 at 10:22 AM, Jordan Rose <<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>> wrote:<br>
><br>
> On Jun 4, 2012, at 1:43 , Daniel Jasper <<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>> wrote:<br>
>> 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.<br>
><br>
> Oh, of course! Good catch.<br>
><br>
>> Could you take one last look as the implementation of IsRecordFullyDefined has changed substantially?<br>
><br>
> Thank you for being careful. :-) Only small comments this time, I think:<br>
><br>
> - Please attach the & to the variable name for references, per LLVM style conventions (same as pointers).<br>
><br>
> - You've got an isa followed by dyn_cast for CXXMethodDecl; please change to just dyn_cast (like CXXRecordDecl).<br>
><br>
> - DenseMap has an operator[], which is a little easier to read than insert+make_pair.<br>
><br>
> - 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.<br>
><br>
> 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.<br>
><br>
> 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.<br>
><br>
> I think that's it. Good work!<br>
><br>
> Jordan<br>
><br>
</div></div><div><div>> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br>