On Wed, Jan 15, 2014 at 12:27 PM, David Majnemer <[email protected]> wrote: > > > On Wednesday, January 15, 2014, Aaron Ballman <[email protected]> > wrote: >> >> Some attribute-related nits below, but aside from those, LGTM (though >> I would get secondary confirmation). >> >> > Index: include/clang/AST/DeclCXX.h >> > =================================================================== >> > --- include/clang/AST/DeclCXX.h >> > +++ include/clang/AST/DeclCXX.h >> > @@ -261,9 +261,7 @@ >> > /// The inheritance model to use for member pointers of a given >> > CXXRecordDecl. >> > enum MSInheritanceModel { >> > MSIM_Single, >> > - MSIM_SinglePolymorphic, >> > MSIM_Multiple, >> > - MSIM_MultiplePolymorphic, >> > MSIM_Virtual, >> > MSIM_Unspecified >> > }; >> > @@ -1609,6 +1607,7 @@ >> > >> > /// \brief Returns the inheritance model used for this record. >> > MSInheritanceModel getMSInheritanceModel() const; >> > + void setMSInheritanceModel(); >> > >> > /// \brief Determine whether this lambda expression was known to be >> > dependent >> > /// at the time it was created, even if its context does not appear >> > to be >> > Index: include/clang/Basic/Attr.td >> > =================================================================== >> > --- include/clang/Basic/Attr.td >> > +++ include/clang/Basic/Attr.td >> > @@ -1354,7 +1354,12 @@ >> > Accessor<"IsUnspecified", [Keyword<"">]>]; >> > // This index is based off the Spellings list and corresponds to the >> > empty >> > // keyword "spelling." >> > - let AdditionalMembers = [{static const int UnspecifiedSpellingIndex = >> > 3;}]; >> > + let AdditionalMembers = [{ >> > + static const unsigned SingleSpellingIndex = 0; >> > + static const unsigned MultipleSpellingIndex = 1; >> > + static const unsigned VirtualSpellingIndex = 2; >> > + static const unsigned UnspecifiedSpellingIndex = 3; >> >> I'd rather to go great length to avoid exposing these further... >> >> let AdditionalMembers = [{ >> // Note: these correspond to the spelling list index for a reason, >> so keep them in sync. >> enum Type { >> Single, >> Multiple, >> Virtual, >> Unspecified >> }; >> >> static MSInheritanceAttr *Create(SourceRange R, ASTContext &C, Type T) { >> return ::new (C) MSInheritanceAttr(R, C, T); >> } >> }]; >> >> This keeps all of the crazy and fragile in one spot. Once you land >> that, I can change the usage in SemaType.cpp over to the new style, >> and remove the old spelling list constant. >> >> > + }]; >> > } >> > >> > def Unaligned : IgnoredAttr { >> > Index: lib/AST/MicrosoftCXXABI.cpp >> > =================================================================== >> > --- lib/AST/MicrosoftCXXABI.cpp >> > +++ lib/AST/MicrosoftCXXABI.cpp >> > @@ -92,7 +92,8 @@ >> > return false; >> > } >> > >> > -static MSInheritanceModel MSInheritanceAttrToModel(MSInheritanceAttr >> > *Attr) { >> > +static MSInheritanceModel >> > +MSInheritanceAttrToModel(const MSInheritanceAttr *Attr) { >> > if (Attr->IsSingle()) >> > return MSIM_Single; >> > else if (Attr->IsMultiple()) >> > @@ -104,16 +105,44 @@ >> > return MSIM_Unspecified; >> > } >> > >> > +static unsigned MSInheritanceModelToSpellingKind(MSInheritanceModel >> > MSIM) { >> > + switch (MSIM) { >> > + case MSIM_Single: >> > + return MSInheritanceAttr::SingleSpellingIndex; >> > + case MSIM_Multiple: >> > + return MSInheritanceAttr::MultipleSpellingIndex; >> > + case MSIM_Virtual: >> > + return MSInheritanceAttr::VirtualSpellingIndex; >> > + case MSIM_Unspecified: >> > + return MSInheritanceAttr::UnspecifiedSpellingIndex; >> > + } >> > +} >> > + >> > + >> > +static MSInheritanceModel calculateInheritanceModel(const CXXRecordDecl >> > *RD) { >> > + if (!RD->hasDefinition()) >> > + return MSIM_Unspecified; >> > + if (RD->getNumVBases() > 0) >> > + return MSIM_Virtual; >> > + if (usesMultipleInheritanceModel(RD)) >> > + return MSIM_Multiple; >> > + return MSIM_Single; >> > +} >> > + >> > MSInheritanceModel CXXRecordDecl::getMSInheritanceModel() const { >> > - if (MSInheritanceAttr *IA = this->getAttr<MSInheritanceAttr>()) >> > + if (MSInheritanceAttr *IA = getAttr<MSInheritanceAttr>()) >> > return MSInheritanceAttrToModel(IA); >> > - // If there was no explicit attribute, the record must be defined >> > already, and >> > - // we can figure out the inheritance model from its other properties. >> > - if (this->getNumVBases() > 0) >> > - return MSIM_Virtual; >> > - if (usesMultipleInheritanceModel(this)) >> > - return this->isPolymorphic() ? MSIM_MultiplePolymorphic : >> > MSIM_Multiple; >> > - return this->isPolymorphic() ? MSIM_SinglePolymorphic : MSIM_Single; >> > + >> > + return calculateInheritanceModel(this); >> > +} >> > + >> > +void CXXRecordDecl::setMSInheritanceModel() { >> > + if (hasAttr<MSInheritanceAttr>()) >> > + return; >> > + >> > + MSInheritanceModel MSIM = calculateInheritanceModel(this); >> > + addAttr(::new (getASTContext()) MSInheritanceAttr( >> > + SourceRange(), getASTContext(), >> > MSInheritanceModelToSpellingKind(MSIM))); >> >> This should be using the source range for the record decl (better >> error reporting if it ever comes up). > > I chose not to because I wanted a way to mark the Attr implicit so that > tooling, AST dumps and diagnostics wouldn't believe there was one in the > source. I can change it to use the record decl's range but could we > (eventually) get a way to say it was manufactured by clang?
That's on "The List" for someday. We have this problem all over the source base (usually I whine about it when I see a new usage, too). > P.S. Personally, I think it would be confusing to use the source range of > this, or any, implicit Attr. The diagnostics wouldn't really make sense to > the user. I can see some logic on that, but then again, at least pointing the user "here's the thing that's affected" would be a kindness. However, it could also be argued that most implicit attributes shouldn't cause errors or warnings in an ideal world. ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
