I guess I'm okay with the extra memory usage. A few last comments:
+bool InitializationHasSideEffects(const FieldDecl& FD) {
Please make this file-scoped (static).
+ // Mark FieldDecl as being used if it is a non-primitive type and the
+ // initializer does not call the default constructor (which is trivial
+ // for all entries in UnusedPrivateFields).
+ // FIXME: Make this smarter once more side effect-free types can be
+ // determined.
+ if (NumArgs > 0) {
+ ValueDecl *VD = cast<ValueDecl>(Member);
+ if (VD->getType()->isRecordType()) {
+ UnusedPrivateFields.remove(VD);
+ } else {
+ for (unsigned i = 0; i < NumArgs; ++i) {
+ if (Args[i]->HasSideEffects(Context)) {
+ UnusedPrivateFields.remove(VD);
+ break;
+ }
+ }
+ }
+ }
'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?
+ } else if (const CXXRecordDecl *R = dyn_cast<CXXRecordDecl>(*I)) {
+ // Check if nested classes are complete.
+ if (R->hasDefinition() &&
+ R->getCanonicalDecl() != RD->getCanonicalDecl())
+ Complete = IsRecordFullyDefined(R, NotFullyDefined, FullyDefined);
+ }
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.
+ const CXXRecordDecl *RD = cast<const CXXRecordDecl>(D->getDeclContext());
+ if (RD && IsRecordFullyDefined(RD, NotFullyDefined, FullyDefined)) {
This cast should be dyn_cast, and the 'const' is unnecessary.
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?
(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.)
Sorry for the review delay. Do you have commit access?
Jordy
On May 22, 2012, at 18:17, Daniel Jasper wrote:
>
>
> On Fri, May 11, 2012 at 12:37 AM, Jordan Rose <[email protected]> wrote:
>
> On May 10, 2012, at 18:12, Daniel Jasper wrote:
>
> > Ok, I now reuse Sema::RecordDeclSetTy.
> >
> > 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.
> >
> > Please find the new version attached.
>
> 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.)
>
> Also, it looks like Decl already has an isUsed() predicate. Are we already
> marking FieldDecls this way?
>
> 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?
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
