[PATCH] Sema: Diagnose improper application of inheritance keywords
Aaron Ballman
aaron.ballman at gmail.com
Wed Jan 29 12:37:23 PST 2014
Patch LGTM! Two very tiny suggestions about the diagnostics below, but
you're welcome to ignore them if you'd prefer.
> Index: include/clang/AST/DeclCXX.h
> ===================================================================
> --- include/clang/AST/DeclCXX.h
> +++ include/clang/AST/DeclCXX.h
> @@ -1602,6 +1602,8 @@
> MSInheritanceAttr::Spelling getMSInheritanceModel() const;
> /// \brief Locks-in the inheritance model for this class.
> void setMSInheritanceModel();
> + /// \brief Calculate what the inheritance model would be for this class.
> + MSInheritanceAttr::Spelling calculateInheritanceModel() const;
>
> /// \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
> @@ -1401,10 +1401,6 @@
> Keyword<"__multiple_inheritance">,
> Keyword<"__virtual_inheritance">,
> Keyword<"__unspecified_inheritance">];
> - let Accessors = [Accessor<"IsSingle", [Keyword<"__single_inheritance">]>,
> - Accessor<"IsMultiple", [Keyword<"__multiple_inheritance">]>,
> - Accessor<"IsVirtual", [Keyword<"__virtual_inheritance">]>,
> - Accessor<"IsUnspecified", [Keyword<"__unspecified_inheritance">]>];
> }
>
> def Unaligned : IgnoredAttr {
> Index: include/clang/Basic/DiagnosticSemaKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticSemaKinds.td
> +++ include/clang/Basic/DiagnosticSemaKinds.td
> @@ -2351,6 +2351,18 @@
> InGroup<DiagGroup<"unsupported-visibility">>;
> def err_mismatched_visibility: Error<"visibility does not match previous declaration">;
> def note_previous_attribute : Note<"previous attribute is here">;
> +def err_mismatched_ms_inheritance : Error<
> + "inheritance model does not match previous declaration">;
> +def warn_ignored_ms_inheritance_on_partial_spec : Warning<
> + "inheritance model ignored on partial specialization">,
> + InGroup<IgnoredAttributes>;
> +def warn_ignored_ms_inheritance_on_primary_template : Warning<
> + "inheritance model ignored on primary template">,
> + InGroup<IgnoredAttributes>;
Maybe combine the two "ignored" attributes into one with a %select?
Not a huge deal, just saves a few bytes.
> +def err_inconsistent_ms_inheritance : Error<
> + "inheritance model does not match definition">;
Same here with the previous "does not match" error.
> +def note_previous_ms_inheritance : Note<
> + "previous inheritance model specified here">;
> def err_machine_mode : Error<"%select{unknown|unsupported}0 machine mode %1">;
> def err_mode_not_primitive : Error<
> "mode attribute only supported for integer and floating-point types">;
> Index: include/clang/Sema/Sema.h
> ===================================================================
> --- include/clang/Sema/Sema.h
> +++ include/clang/Sema/Sema.h
> @@ -1863,6 +1863,10 @@
> unsigned AttrSpellingListIndex);
> DLLExportAttr *mergeDLLExportAttr(Decl *D, SourceRange Range,
> unsigned AttrSpellingListIndex);
> + MSInheritanceAttr *
> + mergeMSInheritanceAttr(Decl *D, SourceRange Range,
> + unsigned AttrSpellingListIndex,
> + MSInheritanceAttr::Spelling SemanticSpelling);
> FormatAttr *mergeFormatAttr(Decl *D, SourceRange Range,
> IdentifierInfo *Format, int FormatIdx,
> int FirstArg, unsigned AttrSpellingListIndex);
> @@ -2572,6 +2576,9 @@
> bool checkStringLiteralArgumentAttr(const AttributeList &Attr,
> unsigned ArgNum, StringRef &Str,
> SourceLocation *ArgLocation = 0);
> + bool checkMSInheritanceAttrOnDefinition(
> + CXXRecordDecl *RD, SourceRange Range,
> + MSInheritanceAttr::Spelling SemanticSpelling);
>
> void CheckAlignasUnderalignment(Decl *D);
>
> Index: lib/AST/MicrosoftCXXABI.cpp
> ===================================================================
> --- lib/AST/MicrosoftCXXABI.cpp
> +++ lib/AST/MicrosoftCXXABI.cpp
> @@ -92,43 +92,29 @@
> return false;
> }
>
> -static MSInheritanceAttr::Spelling
> -MSInheritanceAttrToModel(const MSInheritanceAttr *Attr) {
> - if (Attr->IsSingle())
> - return MSInheritanceAttr::Keyword_single_inheritance;
> - else if (Attr->IsMultiple())
> - return MSInheritanceAttr::Keyword_multiple_inheritance;
> - else if (Attr->IsVirtual())
> - return MSInheritanceAttr::Keyword_virtual_inheritance;
> -
> - assert(Attr->IsUnspecified() && "Expected unspecified inheritance attr");
> - return MSInheritanceAttr::Keyword_unspecified_inheritance;
> -}
> -
> -static MSInheritanceAttr::Spelling
> -calculateInheritanceModel(const CXXRecordDecl *RD) {
> - if (!RD->hasDefinition())
> +MSInheritanceAttr::Spelling CXXRecordDecl::calculateInheritanceModel() const {
> + if (!hasDefinition())
> return MSInheritanceAttr::Keyword_unspecified_inheritance;
> - if (RD->getNumVBases() > 0)
> + if (getNumVBases() > 0)
> return MSInheritanceAttr::Keyword_virtual_inheritance;
> - if (usesMultipleInheritanceModel(RD))
> + if (usesMultipleInheritanceModel(this))
> return MSInheritanceAttr::Keyword_multiple_inheritance;
> return MSInheritanceAttr::Keyword_single_inheritance;
> }
>
> MSInheritanceAttr::Spelling
> CXXRecordDecl::getMSInheritanceModel() const {
> MSInheritanceAttr *IA = getAttr<MSInheritanceAttr>();
> assert(IA && "Expected MSInheritanceAttr on the CXXRecordDecl!");
> - return MSInheritanceAttrToModel(IA);
> + return IA->getSemanticSpelling();
> }
>
> void CXXRecordDecl::setMSInheritanceModel() {
> if (hasAttr<MSInheritanceAttr>())
> return;
>
> addAttr(MSInheritanceAttr::CreateImplicit(
> - getASTContext(), calculateInheritanceModel(this), getSourceRange()));
> + getASTContext(), calculateInheritanceModel(), getSourceRange()));
> }
>
> // Returns the number of pointer and integer slots used to represent a member
> Index: lib/Sema/SemaDecl.cpp
> ===================================================================
> --- lib/Sema/SemaDecl.cpp
> +++ lib/Sema/SemaDecl.cpp
> @@ -1962,6 +1962,9 @@
> else if (SectionAttr *SA = dyn_cast<SectionAttr>(Attr))
> NewAttr = S.mergeSectionAttr(D, SA->getRange(), SA->getName(),
> AttrSpellingListIndex);
> + else if (MSInheritanceAttr *IA = dyn_cast<MSInheritanceAttr>(Attr))
> + NewAttr = S.mergeMSInheritanceAttr(D, IA->getRange(), AttrSpellingListIndex,
> + IA->getSemanticSpelling());
> else if (isa<AlignedAttr>(Attr))
> // AlignedAttrs are handled separately, because we need to handle all
> // such attributes on a declaration at the same time.
> @@ -12104,9 +12107,15 @@
> if (!Completed)
> Record->completeDefinition();
>
> - if (Record->hasAttrs())
> + if (Record->hasAttrs()) {
> CheckAlignasUnderalignment(Record);
>
> + if (MSInheritanceAttr *IA = Record->getAttr<MSInheritanceAttr>())
> + checkMSInheritanceAttrOnDefinition(cast<CXXRecordDecl>(Record),
> + IA->getRange(),
> + IA->getSemanticSpelling());
> + }
> +
> // Check if the structure/union declaration is a type that can have zero
> // size in C. For C this is a language extension, for C++ it may cause
> // compatibility problems.
> Index: lib/Sema/SemaDeclAttr.cpp
> ===================================================================
> --- lib/Sema/SemaDeclAttr.cpp
> +++ lib/Sema/SemaDeclAttr.cpp
> @@ -2872,6 +2872,22 @@
> }
> }
>
> +bool Sema::checkMSInheritanceAttrOnDefinition(
> + CXXRecordDecl *RD, SourceRange Range,
> + MSInheritanceAttr::Spelling SemanticSpelling) {
> + assert(RD->hasDefinition() && "RD has no definition!");
> +
> + if (SemanticSpelling != MSInheritanceAttr::Keyword_unspecified_inheritance &&
> + RD->calculateInheritanceModel() != SemanticSpelling) {
> + Diag(Range.getBegin(), diag::err_inconsistent_ms_inheritance);
> + Diag(RD->getDefinition()->getLocation(), diag::note_defined_here)
> + << RD->getNameAsString();
> + return true;
> + }
> +
> + return false;
> +}
> +
> /// handleModeAttr - This attribute modifies the width of a decl with primitive
> /// type.
> ///
> @@ -3674,6 +3690,19 @@
> Attr.getAttributeSpellingListIndex()));
> }
>
> +static void handleMSInheritanceAttr(Sema &S, Decl *D, const AttributeList &Attr) {
> + if (!S.LangOpts.CPlusPlus) {
> + S.Diag(Attr.getLoc(), diag::err_attribute_not_supported_in_lang)
> + << Attr.getName() << AttributeLangSupport::C;
> + return;
> + }
> + MSInheritanceAttr *IA = S.mergeMSInheritanceAttr(
> + D, Attr.getRange(), Attr.getAttributeSpellingListIndex(),
> + (MSInheritanceAttr::Spelling)Attr.getSemanticSpelling());
> + if (IA)
> + D->addAttr(IA);
> +}
> +
> static void handleARMInterruptAttr(Sema &S, Decl *D,
> const AttributeList &Attr) {
> // Check the attribute arguments.
> @@ -3848,6 +3877,38 @@
> D->addAttr(NewAttr);
> }
>
> +MSInheritanceAttr *
> +Sema::mergeMSInheritanceAttr(Decl *D, SourceRange Range,
> + unsigned AttrSpellingListIndex,
> + MSInheritanceAttr::Spelling SemanticSpelling) {
> + if (MSInheritanceAttr *IA = D->getAttr<MSInheritanceAttr>()) {
> + if (IA->getSemanticSpelling() == SemanticSpelling)
> + return 0;
> + Diag(IA->getLocation(), diag::err_mismatched_ms_inheritance);
> + Diag(Range.getBegin(), diag::note_previous_ms_inheritance);
> + D->dropAttr<MSInheritanceAttr>();
> + }
> +
> + CXXRecordDecl *RD = cast<CXXRecordDecl>(D);
> + if (RD->hasDefinition()) {
> + if (checkMSInheritanceAttrOnDefinition(RD, Range, SemanticSpelling)) {
> + return 0;
> + }
> + } else {
> + if (isa<ClassTemplatePartialSpecializationDecl>(RD)) {
> + Diag(Range.getBegin(), diag::warn_ignored_ms_inheritance_on_partial_spec);
> + return 0;
> + }
> + if (RD->getDescribedClassTemplate()) {
> + Diag(Range.getBegin(), diag::warn_ignored_ms_inheritance_on_primary_template);
> + return 0;
> + }
> + }
> +
> + return ::new (Context)
> + MSInheritanceAttr(Range, Context, AttrSpellingListIndex);
> +}
> +
> /// Handles semantic checking for features that are common to all attributes,
> /// such as checking whether a parameter was properly specified, or the correct
> /// number of arguments were passed, etc.
> @@ -4130,7 +4191,7 @@
> handleUuidAttr(S, D, Attr);
> break;
> case AttributeList::AT_MSInheritance:
> - handleSimpleAttribute<MSInheritanceAttr>(S, D, Attr); break;
> + handleMSInheritanceAttr(S, D, Attr); break;
> case AttributeList::AT_ForceInline:
> handleSimpleAttribute<ForceInlineAttr>(S, D, Attr); break;
> case AttributeList::AT_SelectAny:
> Index: test/SemaCXX/member-pointer-ms.cpp
> ===================================================================
> --- test/SemaCXX/member-pointer-ms.cpp
> +++ test/SemaCXX/member-pointer-ms.cpp
> @@ -117,9 +117,7 @@
>
> static_assert(sizeof(variable_forces_sizing) == kUnspecifiedDataSize, "");
> static_assert(sizeof(MemPtr1) == kUnspecifiedDataSize, "");
> -// FIXME: Clang fails this assert because it locks in the inheritance model at
> -// the point of the typedef instead of the first usage, while MSVC does not.
> -//static_assert(sizeof(MemPtr2) == kSingleDataSize, "");
> +static_assert(sizeof(MemPtr2) == kSingleDataSize, "");
>
> struct MemPtrInBody {
> typedef int MemPtrInBody::*MemPtr;
> @@ -166,3 +164,17 @@
>
> int Virtual::*CastTest = reinterpret_cast<int Virtual::*>(&AA::x);
> // expected-error at -1 {{cannot reinterpret_cast from member pointer type}}
> +
> +namespace ErrorTest {
> +template <typename T, typename U> struct __single_inheritance A;
> + // expected-warning at -1 {{inheritance model ignored on primary template}}
> +template <typename T> struct __multiple_inheritance A<T, T>;
> + // expected-warning at -1 {{inheritance model ignored on partial specialization}}
> +template <> struct __single_inheritance A<int, float>;
> +
> +struct B {}; // expected-note {{B defined here}}
> +struct __multiple_inheritance B; // expected-error{{inheritance model does not match definition}}
> +
> +struct __multiple_inheritance C {}; // expected-error{{inheritance model does not match definition}}
> + // expected-note at -1 {{C defined here}}
> +}
>
~Aaron
On Wed, Jan 29, 2014 at 3:30 PM, David Majnemer
<david.majnemer at gmail.com> wrote:
> Address review comments.
>
> Hi rnk, rsmith, aaron.ballman,
>
> http://llvm-reviews.chandlerc.com/D2585
>
> CHANGE SINCE LAST DIFF
> http://llvm-reviews.chandlerc.com/D2585?vs=6553&id=6752#toc
>
> Files:
> include/clang/AST/DeclCXX.h
> include/clang/Basic/Attr.td
> include/clang/Basic/DiagnosticSemaKinds.td
> include/clang/Sema/Sema.h
> lib/AST/MicrosoftCXXABI.cpp
> lib/Sema/SemaDecl.cpp
> lib/Sema/SemaDeclAttr.cpp
> test/SemaCXX/member-pointer-ms.cpp
More information about the cfe-commits
mailing list