Did/will anyone have time to take a look at this? On Mon, Feb 6, 2012 at 11:04 AM, Daniel Jasper <[email protected]> wrote:
> Comments inline, new version attached. > > On Wed, Feb 1, 2012 at 10:52 AM, Chandler Carruth <[email protected]>wrote: > >> Thanks, comments inline... This is sadly a pretty cursory read, sorry, >> working on other stuff. I think, especially given the "used" concept at >> play here, Richard or Eli looking at this would be very good. =] Thanks >> again for working on it. >> >> --- a/include/clang/Basic/DiagnosticSemaKinds.td >> +++ b/include/clang/Basic/DiagnosticSemaKinds.td >> @@ -134,6 +134,9 @@ def warn_unneeded_member_function : Warning< >> "member function %0 is not needed and will not be emitted">, >> InGroup<UnneededMemberFunction>, DefaultIgnore; >> >> +def warn_unused_private_field: Warning<"private field %0 is not used">, >> + InGroup<UnusedPrivateField>, DefaultIgnore; >> + >> >> I wouldn't separate this in its own group w/ vertical whitespace, but >> rather jam it in with the other unused warning messages. >> > > Done. > > >> --- a/lib/Sema/SemaDeclCXX.cpp >> +++ b/lib/Sema/SemaDeclCXX.cpp >> @@ -1617,8 +1617,25 @@ Sema::ActOnCXXMemberDeclarator(Scope *S, >> AccessSpecifier AS, Declarator &D, >> >> assert((Name || isInstField) && "No identifier for non-field ?"); >> >> - if (isInstField) >> - FieldCollector->Add(cast<FieldDecl>(Member)); >> + if (isInstField) { >> + FieldDecl *FD = cast<FieldDecl>(Member); >> + FieldCollector->Add(FD); >> + if (FD->getAccess() == AS_private) { >> + bool hasNoSideEffects = true; >> + if (!FD->getType().isNull()) { >> + if (const CXXRecordDecl *RD = >> + FD->getType().getTypePtrOrNull()->getAsCXXRecordDecl()) { >> >> You shouldn't need getTypePtrOrNull() here. QualType provides an -> >> overload that delegates to Type. >> > > Done. > > >> >> + if >> (!FD->getParent()->getTypeForDecl()->isInstantiationDependentType() && >> + hasNoSideEffects) >> >> I'm a bit surprised at checking InstantiationDependentType here... Not >> just Dependent... Also some comments as to what this is supposed to handle >> would be good... >> > > Done. > > >> @@ -2063,6 +2080,15 @@ Sema::BuildMemberInitializer(ValueDecl *Member, >> assert((DirectMember || IndirectMember) && >> "Member must be a FieldDecl or IndirectFieldDecl"); >> >> + // FIXME: Allow more initializers not to count as usage, e.g. where all >> + // arguments are primitive types. >> + ValueDecl *VD = dyn_cast<ValueDecl>(Member); >> >> If you know the cast will always succeed, just use "cast" rather than >> "dyn_cast". >> > > Done. > > >> + if (!VD->getType()->isBuiltinType() && >> + !VD->getType()->isAnyPointerType() && >> >> Why are these here? We already have constraints on the type of things >> above when we check the Field Decl? Maybe I just don't understand what >> these are doing. >> > > I added a comment. Basically, this currently marks fields of a > non-primitive type as used, if a non-trivial constructor is called by an > initializer. > > --- a/lib/Sema/SemaTemplateInstantiate.cpp >> +++ b/lib/Sema/SemaTemplateInstantiate.cpp >> @@ -1791,6 +1791,19 @@ Sema::InstantiateClass(SourceLocation >> PointOfInstantiation, >> if (OldField->getInClassInitializer()) >> FieldsWithMemberInitializers.push_back(std::make_pair(OldField, >> Field)); >> + if (Field->getAccess() == AS_private) { >> + bool hasNoSideEffects = true; >> + if (!Field->getType().isNull()) { >> + if (const CXXRecordDecl *RD = >> + >> Field->getType().getTypePtrOrNull()->getAsCXXRecordDecl()) { >> + hasNoSideEffects = RD->isCompleteDefinition() && >> + RD->hasTrivialDefaultConstructor() && >> + RD->hasTrivialDestructor(); >> + } >> + } >> + if (hasNoSideEffects) >> + UnusedPrivateFields.insert(Field); >> + } >> >> This is the second copy of this pattern. We should either have template >> instantiation call the not-template Sema routines to do this, or we should >> factor the predicate into a helper somewhere, likely FieldDecl. >> > > Done. I implemented a method hasSideEffects that computes whether the > declaration of the field has side effects. It currently only recognizes the > trivial default constructor and the trivial destructor as side-effect-free. > This can be extended in the future. Should the method be moved to > DeclaratorDecl or ValueDecl (it doesn't use anything FieldDecl-specific)? >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
