<br><br><div class="gmail_quote">On Mon, Jun 4, 2012 at 10:42 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">
<div>On Mon, Jun 4, 2012 at 12:36 PM, Daniel Jasper <<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>> wrote:<br>
> On Mon, Jun 4, 2012 at 8:33 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>
>> 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>
><br>
><br>
> Do you mean that we should require that as part of this warning (i.e. mark<br>
> those classes as incomplete) or do you mean we should create a separate<br>
> warning for that?<br>
<br>
</div>I mean that we should not consider a class to be completely defined if<br>
it has a pure virtual destructor with no definition in this TU.<br></blockquote><div><br></div><div>Done and test added.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>
> Also, do you think such a destructor might actually have<br>
> side-effects based on the otherwise unused fields. I somewhat doubt that as<br>
> we are only looking at pointers or primitive fields.. We might have<br>
> something like an owning pointer, but there the destructor will never be the<br>
> only "use".<br>
<br>
</div>I'm imagining a case like this:<br>
<br>
// .h<br>
class RAIIBase {<br>
public:<br>
RAIIBase(const char *p) : LogMessage(p) {<br>
my_debug_stream << "Creating " << p;<br>
}<br>
virtual ~RAIIBase() = 0;<br>
private:<br>
const char *LogMessage;<br>
};<br>
<br>
// .cc<br>
RAIIBase::~RAIIBase() {<br>
my_debug_stream << "Destroying " << LogMessage;<br>
}<br></blockquote><div><br></div><div>I wonder, whether this is a case that really ever occurs (due to multiple rare things being used at the same time). But ok.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
If I've understood correctly, the initialization of LogMessage won't<br>
count as a 'use' because it has no side-effects, so any TU including<br>
the .h file (other than the corresponding .cc file) would flag<br>
'LogMessage' as being unused.<br>
<div><br>
>> * 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>
><br>
><br>
> Just to clarify: An in-class initializer is only used, if at least one<br>
> constructor does not have a corresponding member initializer?<br>
<br>
</div>There are some additional complications if the class is a union or has<br>
an anonymous union member. Perhaps moving the marking-as-used for<br>
in-class initializers to CollectFieldInitializer would be best.<br></blockquote><div><br></div><div>Moved this to CollectFieldInitializer. Added call to HasSideEffects (+test) for symmetry with constructor initializers.</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br>
>> * 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>
><br>
><br>
> I'd like to get the first version in. Thus I'd like to add this test case<br>
> together with a FIXME. The correct solution for this (and especially for<br>
> anonymous structs) would be to look at the corresponding DeclContext, right?<br>
<br>
</div>Yes, to find the outer struct, you can walk through the parent<br>
DeclContexts (while they're RecordDecls and<br>
RecordDecl::isAnonymousStructOrUnion returns true).</blockquote><div><br></div><div>Ok, I will do this once the first patch is in as I am not overly concerned about this kind of false negative.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>
>> (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>
><br>
><br>
> Why should we not warn in this case? I think in general we might want to<br>
> exempt char-arrays as they are commonly use for alignment, but other than<br>
> that?<br>
<br>
</div>Aligner is 'unused', but has a semantic effect. Note that in the<br>
clang::DependentFunctionTempateSpecializationInfo case, there is no<br>
char array. I'm concerned that a warning for cases like this would<br>
have a high false positive rate. (I have the same concern for padding<br>
members in structs, but I would expect those to generally be public,<br>
since such structs are usually intended to be PODs.)<br></blockquote><div><br></div><div>Ah, (things-I-know-about-C++)++. :-)</div><div>I know exclude unions from this analysis. </div><div><br></div><div>New patch attached.</div>
</div><br>