[clang] Don't consider a nullptr returned from ActOnTag() as a valid result in ParseClassSpecifier.

[hokein]

Summary

This is revealed in https://reviews.llvm.org/D141280

[hokein]

I think the diagnostic changes are improvements.

clang/test/SemaCXX/rdar42746401.cpp

7

The removed diagnostic is "cannot combine with previous 'type-name' declaration specifier" which is bogus.

[shafik]

I wonder if ActionResult needs a overload that takes std::nullptr_t the current interface seems easy to use wrong like we have seen here.

@aaron.ballman wdyt?

[aaron.ballman]

In D141580#4047737, @shafik wrote:

I wonder if ActionResult needs a overload that takes std::nullptr_t the current interface seems easy to use wrong like we have seen here.

@aaron.ballman wdyt?

I don't think that'll help in this case because the ActionResult is being constructed from a pointer (which happens to be null) and not a nullptr constant.

clang/lib/Parse/ParseDeclCXX.cpp

2069–2078 ↗

(On Diff #488526)

I think this is the wrong approach to fixing the issue. None of the other assignments to TagOrTempResult test for nullptr first, such as: https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2037 and https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2029.

An ActionResult can be both valid/invalid *and* usable/unusable (and a DeclResult is a kind of ActionResult). When it's assigned *any pointer value* (including nullptr), it's a valid ActionResult but it's not a usable ActionResult (https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Sema/Ownership.h#L166).

I think the correct fix is to find the places assuming a valid ActionResult means a nonnull pointer from get(), such as https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2121 and switch them over to looking for a usable ActionResult instead.

[hokein]

clang/lib/Parse/ParseDeclCXX.cpp

2069–2078 ↗

(On Diff #488526)

Thanks for the comment!

None of the other assignments to TagOrTempResult test for nullptr first

I think the problem here is:

• most of the ActOn* methods return a DeclResult (and inside these method implementations, they return an Invalid DeclResult if the Decl is nullptr), so TagOrTempResult = Actions.ActOn* is a fine pattern.
• Only two exceptions ActOnTagDecl and ActOnTemplatedFriendTag, they return a Decl*, so the TagOrTempResult = Action.ActOn* is a very suspicious pattern. (IMO, they're not intentional, likely bugs).

such as: https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2037 and https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2029.

The first one is the other exception; the second one is not (ActOnExplicitInstantiation returns a DeclResult).

I think the correct fix is to find the places assuming a valid ActionResult means a nonnull pointer from get(), such as https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2121 and switch them over to looking for a usable ActionResult instead.

This is one of the solutions. But instead of justifying every places of DeclResult, I think we should fix the two exception places (by adding the nullptr check in
https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2037 and https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2069), what do you think?

[hokein]

Check for the ActOnTemplatedFriendTag.

[hokein]

clang/lib/Parse/ParseDeclCXX.cpp

2069–2078 ↗

(On Diff #488526)

I have updated the patch to check nullptr for ActOnTemplatedFriendTag as well. Let me know what you think (I'm also happy to do the change).

[aaron.ballman]

clang/lib/Parse/ParseDeclCXX.cpp

2069–2078 ↗

(On Diff #488526)

Actually, I think the best solution is to make those APIs return a correctly constructed (invalid) DeclResult rather than a Decl * that turns into a valid-but-unusable DeclResult. I tried the change out locally and it seems to have the same fallout as your changes with only a small amount of extra work involved.

I have this done locally, would you like me to go ahead and clean it up to commit it or hand a patch off to you to finish? Or do you think that's not a good approach?

[sammccall]

To offer the opposing argument: if DeclResult is just a bad idea, then using it consistently/right might be worse than using it as little as possible.

FWIW, I find every piece of code that produces/consumes the *Result to be extremely confusing (the semantics of the two different sentinel values are never obvious) and use of the type in more APIs means more callers have to deal with this tri-state logic and bugs like this one.

So spreading DeclResult around where it's not strictly needed feels like the wrong direction to be, and i find the original version of the patch easier to understand. It may even be possible to switch entirely to pointers here.

(Aaron, this is your call, just wanted to make sure you're aware of the cost)

clang/lib/Parse/ParseDeclCXX.cpp

2069 ↗

(On Diff #488526)

just so I understand:

initial state of TagOrTempResult is (null, invalid)
if ActOnTag returns ptr, we set it to (ptr, valid)
if ActOnTag returns null, then the old code would set it to (null, valid) and the new code would leave it as (null, invalid)

ActOnTag returns null in error cases (diagnostic has been emitted).
We check validity below when deciding whether to use the returned pointer (so we're passing null around at that point).

I can't really infer what the invariants here were originally meant to be, and obviously we're not crashing all over the place, and the diagnostics look like a wash to me. But the new logic seems much more sane.

clang/test/SemaCXX/rdar42746401.cpp

7

That doesn't sound bogus to me: ::b<0> is a type name, struct c::d is a type name, and you can't combine two type-names in a decl-specifier-sequence.

On the other hand it's a follow-on diagnostic in a situation where it's probably not useful.

[hokein]

In D141580#4052496, @sammccall wrote:

To offer the opposing argument: if DeclResult is just a bad idea, then using it consistently/right might be worse than using it as little as possible.

FWIW, I find every piece of code that produces/consumes the *Result to be extremely confusing (the semantics of the two different sentinel values are never obvious) and use of the type in more APIs means more callers have to deal with this tri-state logic and bugs like this one.

So spreading DeclResult around where it's not strictly needed feels like the wrong direction to be, and i find the original version of the patch easier to understand. It may even be possible to switch entirely to pointers here.

Sema::Act* have a mixed usage of returning {*Result`, void, a pointer, bool}, unclear the common practice there. Changing ActOnTemplatedFriendTag and ActOnTag return-type seems be more aligned with the other Sema::Act* methods used in the ParseClassSpecifier context. And these two APIs are not widely used, it is probably fine.

(Aaron, this is your call, just wanted to make sure you're aware of the cost)

To me, I don't have a strong preference, I'm fine with either solution.

clang/lib/Parse/ParseDeclCXX.cpp

2069–2078 ↗

(On Diff #488526)

Actually, I think the best solution is to make those APIs return a correctly constructed (invalid) DeclResult rather than a Decl * that turns into a valid-but-unusable DeclResult. I tried the change out locally and it seems to have the same fallout as your changes with only a small amount of extra work involved.

Yeah, I considered this approach, but I was not convinced myself is significantly better than the current approach, and it requires some more changes.

I have this done locally, would you like me to go ahead and clean it up to commit it or hand a patch off to you to finish? Or do you think that's not a good approach?

Thanks, since you already have a patch, please go ahead (if you think it is better). I'm happy to review.

[aaron.ballman]

In D141580#4052496, @sammccall wrote:

To offer the opposing argument: if DeclResult is just a bad idea, then using it consistently/right might be worse than using it as little as possible.

FWIW, I find every piece of code that produces/consumes the *Result to be extremely confusing (the semantics of the two different sentinel values are never obvious) and use of the type in more APIs means more callers have to deal with this tri-state logic and bugs like this one.

So spreading DeclResult around where it's not strictly needed feels like the wrong direction to be, and i find the original version of the patch easier to understand. It may even be possible to switch entirely to pointers here.

(Aaron, this is your call, just wanted to make sure you're aware of the cost)

FWIW, I also find the interface to be horribly confusing and would love to replace it with something better at some point. But we're horribly inconsistent with things at the moment (some return a pointer, some return a bool, some return a result, etc), so my thinking is that any amount of unification is probably a good thing if only because it makes it more likely we can (consistently) swap to something better in the future.

clang/lib/Parse/ParseDeclCXX.cpp

2069–2078 ↗

(On Diff #488526)

heh, yeah, it's hard to know what the right approach is -- I suspect anything other than what we're doing today is a good step forward. I'll go ahead and make this change; it's straightforward enough I think post-commit review should suffice.

[aeubanks]

apologies for reverting this, but https://reviews.llvm.org/rGf1f0a0d8e8fdd2e534d9423b2e64c6b8aaa53aee is broken and doesn't revert cleanly without this also being reverted, could you reland?

[hokein]

In D141580#4061205, @aeubanks wrote:

apologies for reverting this, but https://reviews.llvm.org/rGf1f0a0d8e8fdd2e534d9423b2e64c6b8aaa53aee is broken and doesn't revert cleanly without this also being reverted, could you reland?

I relanded it in 6898d8413ff4af205000eab1db3fa900b39c6097.

clang/include/clang/Sema/Sema.h

enum OffsetOfKind {
     OOK_Outside,
     // Parsing a type within __builtin_offsetof.
     OOK_Builtin,
     // Parsing a type within macro "offsetof", defined in __buitin_offsetof
     // To improve our diagnostic message.
     OOK_Macro,
   };
 
-  Decl *ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
-                 SourceLocation KWLoc, CXXScopeSpec &SS, IdentifierInfo *Name,
-                 SourceLocation NameLoc, const ParsedAttributesView &Attr,
-                 AccessSpecifier AS, SourceLocation ModulePrivateLoc,
-                 MultiTemplateParamsArg TemplateParameterLists, bool &OwnedDecl,
-                 bool &IsDependent, SourceLocation ScopedEnumKWLoc,
+  DeclResult ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
+                      SourceLocation KWLoc, CXXScopeSpec &SS,
+                      IdentifierInfo *Name, SourceLocation NameLoc,
+                      const ParsedAttributesView &Attr, AccessSpecifier AS,
+                      SourceLocation ModulePrivateLoc,
+                      MultiTemplateParamsArg TemplateParameterLists,
+                      bool &OwnedDecl, bool &IsDependent,
+                      SourceLocation ScopedEnumKWLoc,
                       bool ScopedEnumUsesClassTag, TypeResult UnderlyingType,
                       bool IsTypeSpecifier, bool IsTemplateParamOrArg,
                       OffsetOfKind OOK, SkipBodyInfo *SkipBody = nullptr);
 
-  Decl *ActOnTemplatedFriendTag(Scope *S, SourceLocation FriendLoc,
+  DeclResult ActOnTemplatedFriendTag(Scope *S, SourceLocation FriendLoc,
                                      unsigned TagSpec, SourceLocation TagLoc,
                                      CXXScopeSpec &SS, IdentifierInfo *Name,
                                      SourceLocation NameLoc,
                                      const ParsedAttributesView &Attr,
                                      MultiTemplateParamsArg TempParamLists);
 
   TypeResult ActOnDependentTag(Scope *S,
                                unsigned TagSpec,
                                TagUseKind TUK,
                                const CXXScopeSpec &SS,
                                IdentifierInfo *Name,
                                SourceLocation TagLoc,
                                SourceLocation NameLoc);

clang/lib/Parse/ParseDecl.cpp

void Parser::ParseEnumSpecifier(SourceLocation StartLoc, DeclSpec &DS,
   unsigned DiagID;
   Decl *TagDecl = Actions.ActOnTag(
       getCurScope(), DeclSpec::TST_enum, TUK, StartLoc, SS, Name, NameLoc,
       attrs, AS, DS.getModulePrivateSpecLoc(), TParams, Owned, IsDependent,
       ScopedEnumKWLoc, IsScopedUsingClassTag, BaseType,
       DSC == DeclSpecContext::DSC_type_specifier,
       DSC == DeclSpecContext::DSC_template_param ||
           DSC == DeclSpecContext::DSC_template_type_arg,
-      OffsetOfState, &SkipBody);
+      OffsetOfState, &SkipBody).get();
 
   if (SkipBody.ShouldSkip) {
     assert(TUK == Sema::TUK_Definition && "can only skip a definition");
 
     BalancedDelimiterTracker T(*this, tok::l_brace);
     T.consumeOpen();
     T.skipToEnd();
 

clang/lib/Sema/SemaDecl.cpp

 /// TagSpec indicates what kind of tag this is. TUK indicates whether this is a
 /// reference/declaration/definition of a tag.
 ///
 /// \param IsTypeSpecifier \c true if this is a type-specifier (or
 /// trailing-type-specifier) other than one in an alias-declaration.
 ///
 /// \param SkipBody If non-null, will be set to indicate if the caller should
 /// skip the definition of this tag and treat it as if it were a declaration.
-Decl *Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
-                     SourceLocation KWLoc, CXXScopeSpec &SS,
-                     IdentifierInfo *Name, SourceLocation NameLoc,
-                     const ParsedAttributesView &Attrs, AccessSpecifier AS,
-                     SourceLocation ModulePrivateLoc,
-                     MultiTemplateParamsArg TemplateParameterLists,
-                     bool &OwnedDecl, bool &IsDependent,
-                     SourceLocation ScopedEnumKWLoc,
-                     bool ScopedEnumUsesClassTag, TypeResult UnderlyingType,
-                     bool IsTypeSpecifier, bool IsTemplateParamOrArg,
-                     OffsetOfKind OOK, SkipBodyInfo *SkipBody) {
+DeclResult
+Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK, SourceLocation KWLoc,
+               CXXScopeSpec &SS, IdentifierInfo *Name, SourceLocation NameLoc,
+               MultiTemplateParamsArg TemplateParameterLists, bool &OwnedDecl,
+               bool &IsDependent, SourceLocation ScopedEnumKWLoc,
   // If this is not a definition, it must have a name.
   IdentifierInfo *OrigName = Name;
   assert((Name != nullptr || TUK == TUK_Definition) &&
          "Nameless record must be a definition!");
   assert(TemplateParameterLists.size() == 0 || TUK != TUK_Reference);
 
   OwnedDecl = false;
   TagTypeKind Kind = TypeWithKeyword::getTagTypeKindForTypeSpec(TagSpec);
Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK, SourceLocation KWLoc,
   if (TemplateParameterLists.size() > 0 ||
       (SS.isNotEmpty() && TUK != TUK_Reference)) {
     if (TemplateParameterList *TemplateParams =
             MatchTemplateParametersToScopeSpecifier(
                 KWLoc, NameLoc, SS, nullptr, TemplateParameterLists,
                 TUK == TUK_Friend, isMemberSpecialization, Invalid)) {
       if (Kind == TTK_Enum) {
         Diag(KWLoc, diag::err_enum_template);
-        return nullptr;
+        return true;
       }
 
       if (TemplateParams->size() > 0) {
         // This is a declaration or definition of a class template (which may
         // be a member of another template).
 
         if (Invalid)
-          return nullptr;
+          return true;
 
         OwnedDecl = false;
         DeclResult Result = CheckClassTemplate(
             S, TagSpec, TUK, KWLoc, SS, Name, NameLoc, Attrs, TemplateParams,
             AS, ModulePrivateLoc,
             /*FriendLoc*/ SourceLocation(), TemplateParameterLists.size() - 1,
             TemplateParameterLists.data(), SkipBody);
         return Result.get();
       } else {
         // The "template<>" header is extraneous.
         Diag(TemplateParams->getTemplateLoc(), diag::err_template_tag_noparams)
           << TypeWithKeyword::getTagTypeKindName(Kind) << Name;
         isMemberSpecialization = true;
       }
     }
 
     if (!TemplateParameterLists.empty() && isMemberSpecialization &&
         CheckTemplateDeclScope(S, TemplateParameterLists.back()))
-      return nullptr;
+      return true;
   }
 
   // Figure out the underlying type if this a enum declaration. We need to do
   // this early, because it's needed to detect if this is an incompatible
   // redeclaration.
   llvm::PointerUnion<const Type*, TypeSourceInfo*> EnumUnderlying;
   bool IsFixed = !UnderlyingType.isUnset() || ScopedEnum;
 
if (Name && SS.isNotEmpty()) {
     }
 
     // If this is a friend or a reference to a class in a dependent
     // context, don't try to make a decl for it.
     if (TUK == TUK_Friend || TUK == TUK_Reference) {
       DC = computeDeclContext(SS, false);
       if (!DC) {
         IsDependent = true;
-        return nullptr;
+        return true;
       }
     } else {
       DC = computeDeclContext(SS, true);
       if (!DC) {
         Diag(SS.getRange().getBegin(), diag::err_dependent_nested_name_spec)
           << SS.getRange();
-        return nullptr;
+        return true;
       }
     }
 
     if (RequireCompleteDeclContext(SS, DC))
-      return nullptr;
+      return true;
 
     SearchDC = DC;
     // Look-up name inside 'foo::'.
     LookupQualifiedName(Previous, DC);
 
     if (Previous.isAmbiguous())
-      return nullptr;
+      return true;
 
     if (Previous.empty()) {
       // Name lookup did not find anything. However, if the
       // nested-name-specifier refers to the current instantiation,
       // and that current instantiation has any dependent base
       // classes, we might find something at instantiation time: treat
       // this as a dependent elaborated-type-specifier.
       // But this only makes any sense for reference-like lookups.
       if (Previous.wasNotFoundInCurrentInstantiation() &&
           (TUK == TUK_Reference || TUK == TUK_Friend)) {
         IsDependent = true;
-        return nullptr;
+        return true;
       }
 
       // A tag 'foo::bar' must already exist.
       Diag(NameLoc, diag::err_not_tag_in_scope)
         << Kind << Name << DC << SS.getRange();
       Name = nullptr;
       Invalid = true;
       goto CreateNewDecl;
     }
   } else if (Name) {
     // C++14 [class.mem]p14:
     //   If T is the name of a class, then each of the following shall have a
     //   name different from T:
     //    -- every member of class T that is itself a type
     if (TUK != TUK_Reference && TUK != TUK_Friend &&
         DiagnoseClassNameShadow(SearchDC, DeclarationNameInfo(Name, NameLoc)))
-      return nullptr;
+      return true;
 
     // If this is a named struct, check to see if there was a previous forward
     // declaration or definition.
     // FIXME: We're looking into outer scopes here, even when we
     // shouldn't be. Doing so can result in ambiguities that we
     // shouldn't be diagnosing.
     LookupName(Previous, S);
 
if (!Previous.empty() && TUK == TUK_Friend) {
         NamedDecl *ND = Previous.getFoundDecl();
         Diag(NameLoc, diag::ext_friend_tag_redecl_outside_namespace)
             << createFriendTagNNSFixIt(*this, ND, S, NameLoc);
       }
     }
 
     // Note:  there used to be some attempt at recovery here.
     if (Previous.isAmbiguous())
-      return nullptr;
+      return true;
 
     if (!getLangOpts().CPlusPlus && TUK != TUK_Reference) {
       // FIXME: This makes sure that we ignore the contexts associated
       // with C structs, unions, and enums when looking for a matching
       // tag declaration or definition. See the similar lookup tweak
       // in Sema::LookupName; is there a better way to deal with this?
       while (isa<RecordDecl, EnumDecl, ObjCContainerDecl>(SearchDC))
         SearchDC = SearchDC->getParent();
CreateNewDecl:
 
   OwnedDecl = true;
   // In C++, don't return an invalid declaration. We can't recover well from
   // the cases where we make the type anonymous.
   if (Invalid && getLangOpts().CPlusPlus) {
     if (New->isBeingDefined())
       if (auto RD = dyn_cast<RecordDecl>(New))
         RD->completeDefinition();
-    return nullptr;
+    return true;
   } else if (SkipBody && SkipBody->ShouldSkip) {
     return SkipBody->Previous;
   } else {
     return New;
   }
 }
 
 void Sema::ActOnTagStartDefinition(Scope *S, Decl *TagD) {

clang/lib/Sema/SemaDeclCXX.cpp

FriendDecl *Sema::CheckFriendTypeDecl(SourceLocation LocStart,
   //   the friend declaration is ignored.
   return FriendDecl::Create(Context, CurContext,
                             TSInfo->getTypeLoc().getBeginLoc(), TSInfo,
                             FriendLoc);
 }
 
 /// Handle a friend tag declaration where the scope specifier was
 /// templated.
-Decl *Sema::ActOnTemplatedFriendTag(Scope *S, SourceLocation FriendLoc,
-                                    unsigned TagSpec, SourceLocation TagLoc,
-                                    CXXScopeSpec &SS, IdentifierInfo *Name,
-                                    SourceLocation NameLoc,
-                                    const ParsedAttributesView &Attr,
-                                    MultiTemplateParamsArg TempParamLists) {
+DeclResult Sema::ActOnTemplatedFriendTag(
+    Scope *S, SourceLocation FriendLoc, unsigned TagSpec, SourceLocation TagLoc,
+    CXXScopeSpec &SS, IdentifierInfo *Name, SourceLocation NameLoc,
+    const ParsedAttributesView &Attr, MultiTemplateParamsArg TempParamLists) {
   TagTypeKind Kind = TypeWithKeyword::getTagTypeKindForTypeSpec(TagSpec);
 
   bool IsMemberSpecialization = false;
   bool Invalid = false;
 
   if (TemplateParameterList *TemplateParams =
           MatchTemplateParametersToScopeSpecifier(
               TagLoc, NameLoc, SS, nullptr, TempParamLists, /*friend*/ true,
               IsMemberSpecialization, Invalid)) {
     if (TemplateParams->size() > 0) {
       // This is a declaration of a class template.
       if (Invalid)
-        return nullptr;
+        return true;
 
       return CheckClassTemplate(S, TagSpec, TUK_Friend, TagLoc, SS, Name,
                                 NameLoc, Attr, TemplateParams, AS_public,
                                 /*ModulePrivateLoc=*/SourceLocation(),
                                 FriendLoc, TempParamLists.size() - 1,
                                 TempParamLists.data()).get();
     } else {
       // The "template<>" header is extraneous.
       Diag(TemplateParams->getTemplateLoc(), diag::err_template_tag_noparams)
         << TypeWithKeyword::getTagTypeKindName(Kind) << Name;
       IsMemberSpecialization = true;
     }
   }
 
-  if (Invalid) return nullptr;
+  if (Invalid) return true;
 
   bool isAllExplicitSpecializations = true;
   for (unsigned I = TempParamLists.size(); I-- > 0; ) {
     if (TempParamLists[I]->size()) {
       isAllExplicitSpecializations = false;
       break;
     }
   }
if (isAllExplicitSpecializations) {
     }
 
     NestedNameSpecifierLoc QualifierLoc = SS.getWithLocInContext(Context);
     ElaboratedTypeKeyword Keyword
       = TypeWithKeyword::getKeywordForTagTypeKind(Kind);
     QualType T = CheckTypenameType(Keyword, TagLoc, QualifierLoc,
                                    *Name, NameLoc);
     if (T.isNull())
-      return nullptr;
+      return true;
 
     TypeSourceInfo *TSI = Context.CreateTypeSourceInfo(T);
     if (isa<DependentNameType>(T)) {
       DependentNameTypeLoc TL =
           TSI->getTypeLoc().castAs<DependentNameTypeLoc>();
       TL.setElaboratedKeywordLoc(TagLoc);
       TL.setQualifierLoc(QualifierLoc);
       TL.setNameLoc(NameLoc);

clang/lib/Sema/SemaTemplate.cpp

Sema::ActOnExplicitInstantiation(Scope *S, SourceLocation ExternLoc,
 
   bool Owned = false;
   bool IsDependent = false;
   Decl *TagD = ActOnTag(
       S, TagSpec, Sema::TUK_Reference, KWLoc, SS, Name, NameLoc, Attr, AS_none,
       /*ModulePrivateLoc=*/SourceLocation(), MultiTemplateParamsArg(), Owned,
       IsDependent, SourceLocation(), false, TypeResult(),
       /*IsTypeSpecifier*/ false,
-      /*IsTemplateParamOrArg=*/false, /*OOK=*/OOK_Outside);
+      /*IsTemplateParamOrArg=*/false, /*OOK=*/OOK_Outside).get();
   assert(!IsDependent && "explicit instantiation of dependent name not yet handled");
 
   if (!TagD)
     return true;
 
   TagDecl *Tag = cast<TagDecl>(TagD);
   assert(!Tag->isEnum() && "shouldn't see enumerations here");
 

clang/test/Parser/cxx-undeclared-identifier.cpp

struct S {
     void f();
   };
   S::f() {} // expected-error {{a type specifier is required}}
 }
 
 // PR7180
 int f(a::b::c); // expected-error {{use of undeclared identifier 'a'}}
 
-class Foo::Bar { // expected-error {{use of undeclared identifier 'Foo'}} \
-                 // expected-error {{expected ';' after class}}
+class Foo::Bar { // expected-error {{use of undeclared identifier 'Foo'}}
+                 // expected-error {{expected unqualified-id}}

clang/test/Parser/recovery.cpp

 }
 
 namespace InvalidEmptyNames {
 // These shouldn't crash, the diagnostics aren't important.
 struct ::, struct ::; // expected-error 2 {{expected identifier}} expected-error 2 {{declaration of anonymous struct must be a definition}} expected-warning {{declaration does not declare anything}}
 enum ::, enum ::; // expected-error 2 {{expected identifier}}
 struct ::__super, struct ::__super; // expected-error 2 {{expected identifier}} expected-error 2 {{expected '::' after '__super'}}
 struct ::template foo, struct ::template bar; // expected-error 2 {{expected identifier}} expected-error 2 {{declaration of anonymous struct must be a definition}} expected-warning {{declaration does not declare anything}}
-struct ::foo struct::; // expected-error {{no struct named 'foo' in the global namespace}} expected-error {{expected identifier}} expected-error {{declaration of anonymous struct must be a definition}}
+struct ::foo struct::; // expected-error {{no struct named 'foo' in the global namespace}} expected-error {{expected identifier}}
 class :: : {} a;  // expected-error {{expected identifier}} expected-error {{expected class name}}
 }

clang/test/SemaCXX/invalid-template-params.cpp

 };
 
 Foo<int>::UBar g1; // expected-error {{no type named 'UBar' in 'Foo<int>'}}
 
 class C0 {
 public:
   template<typename T0, typename T1 = T0 // missing closing angle bracket
   struct S0 {}; // expected-error {{'S0' cannot be defined in a type specifier}}
-                // expected-error@-1 {{cannot combine with previous 'type-name' declaration specifier}}
-                // expected-error@-2 {{expected ',' or '>' in template-parameter-list}}
-                // expected-error@-3 {{declaration does not declare anything}}
+                // expected-error@-1 {{expected ',' or '>' in template-parameter-list}}
+                // expected-error@-2 {{declaration does not declare anything}}
   C0() : m(new S0<int>) {} // expected-error {{expected '(' for function-style cast or type construction}}
                            // expected-error@-1 {{expected expression}}
   S0<int> *m; // expected-error {{expected member name or ';' after declaration specifiers}}
 };

clang/test/SemaCXX/rdar42746401.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s
 
 template <int>
 class b;
 class c; // expected-note{{forward declaration}}
 
-::b<0> struct c::d // expected-error{{incomplete type}} expected-error{{cannot combine}} expected-error{{expected unqualified-id}}
+::b<0> struct c::d // expected-error{{incomplete type}} expected-error{{expected unqualified-id}}