<br><br><div class="gmail_quote">On Sun, May 27, 2012 at 2:31 AM, Jordy Rose <span dir="ltr"><<a href="mailto:jediknil@belkadan.com" target="_blank">jediknil@belkadan.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I guess I'm okay with the extra memory usage. A few last comments:<br>
<br>
+bool InitializationHasSideEffects(const FieldDecl& FD) {<br>
<br>
Please make this file-scoped (static).</blockquote><div> </div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
+ // Mark FieldDecl as being used if it is a non-primitive type and the<br>
+ // initializer does not call the default constructor (which is trivial<br>
+ // for all entries in UnusedPrivateFields).<br>
+ // FIXME: Make this smarter once more side effect-free types can be<br>
+ // determined.<br>
</div>+ if (NumArgs > 0) {<br>
<div>+ ValueDecl *VD = cast<ValueDecl>(Member);<br>
</div>+ if (VD->getType()->isRecordType()) {<br>
+ UnusedPrivateFields.remove(VD);<br>
+ } else {<br>
+ for (unsigned i = 0; i < NumArgs; ++i) {<br>
+ if (Args[i]->HasSideEffects(Context)) {<br>
+ UnusedPrivateFields.remove(VD);<br>
+ break;<br>
+ }<br>
+ }<br>
+ }<br>
+ }<br>
<br>
'Member' is already typed as a ValueDecl. Also, even if one of the constructor arguments to a primitive type has side effects, shouldn't the field still count as unused if it's never referenced again?<br>
</blockquote><div><br></div><div>Removed VD.</div><div><br></div><div>I personally agree, but I have also heard that some people do initialize fields with methods that have side effects and use the value only with certain #ifdefs (e.g. for debugging). In these cases, the warning would be annoying. Thus, in order to reduce false positives as far as possible in this first version, I declared this as a "use". I am happy to change it, though.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ } else if (const CXXRecordDecl *R = dyn_cast<CXXRecordDecl>(*I)) {<br>
+ // Check if nested classes are complete.<br>
+ if (R->hasDefinition() &&<br>
+ R->getCanonicalDecl() != RD->getCanonicalDecl())<br>
+ Complete = IsRecordFullyDefined(R, NotFullyDefined, FullyDefined);<br>
+ }<br>
<br>
This clause in IsRecordFullyDefined seems a little fishy to me. If a nested class does NOT have a definition, the enclosing class is still complete? If it has a definition but the canonical decl is the current class -- okay, I get that, if the class has a reference to itself.</blockquote>
<div><br></div><div>Yes, you are right. I actually wanted to not check the injected class name but I messed it up (and didn't find isInjectedClassname). Also, it did not correctly handle cases where the inner class was defined outside of the outer class. Fixed and tests added.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
+ const CXXRecordDecl *RD = cast<const CXXRecordDecl>(D->getDeclContext());<br>
</div>+ if (RD && IsRecordFullyDefined(RD, NotFullyDefined, FullyDefined)) {<br>
<br>
This cast should be dyn_cast, and the 'const' is unnecessary.<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Finally, even though there's not a big hit traversing the fields at the end, we probably still shouldn't pay for this if the diagnostic is off. Can you check for that before inserting into UnusedPrivateFields, or even doing all the tests?<br>
</blockquote><div><br></div><div>I am now checking whether the diagnostic is enabled before inserting into the set and before doing the checks at the end of the translation unit. Checking before removing from the (then empty) set probably does not give any benefit. I am not sure whether the check on the initializer arguments is expensive (especially the call to HasSideEffects). What do you think?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
(Oh, and how does this do with private static members? I think you'd be missing a case there. If you want to just exclude them for now, though, that's okay.)<br></blockquote><div><br></div><div>I added a static private member to the test. The warning does not fire at the moment and I think that is ok for the first version.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Sorry for the review delay. Do you have commit access?</blockquote><div><br></div><div>Yes, as of today :-). Please find a new version of the patch attached.</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">
<span><font color="#888888"><br>
Jordy<br>
</font></span><div><div><br>
<br>
On May 22, 2012, at 18:17, Daniel Jasper wrote:<br>
<br>
><br>
><br>
> On Fri, May 11, 2012 at 12:37 AM, Jordan Rose <<a href="mailto:jediknil@belkadan.com" target="_blank">jediknil@belkadan.com</a>> wrote:<br>
><br>
> On May 10, 2012, at 18:12, Daniel Jasper wrote:<br>
><br>
> > Ok, I now reuse Sema::RecordDeclSetTy.<br>
> ><br>
> > As for putting a bit into FieldDecl as opposed to the UnusedPrivateField SetVector: It seems like FieldDecl is carefully crafted for size. Just adding another bit for this use case doesn't seem right.<br>
> ><br>
> > Please find the new version attached.<br>
><br>
> CachedFieldIndex is just the index of the field in the record; there's no way that'll reach 2^31 anyway. (For comparison, C++11 [implimits] suggests a minimum of 4096 members.)<br>
><br>
> Also, it looks like Decl already has an isUsed() predicate. Are we already marking FieldDecls this way?<br>
><br>
> I have been looking into this some more. Marking the fields as used works fine, but I didn't find an easy way to iterate over all of the FieldDecls in a translation unit at the end. Thus, I suggest to stick with the previous version of my patch. Should also neither be a big performance nor memory impact. What do you think?<br>
<br>
</div></div></blockquote></div><br>