[Clang] Don't tell people to place _Alignas on a struct in diagnostics

[theo-lw]

Summary

This changes the diagnostic for _Alignas so it no longer tells people to place _Alignas after the "struct" keyword.

Fixes https://github.com/llvm/llvm-project/issues/58637

[theo-lw]

Accidentally deleted a line causing this to not compile :(

[theo-lw]

Didn't realize this test was failing so I've updated it.

[aaron.ballman]

Thank you for the fix, please be sure to add a release note for the fix as well. Also, I'd like to see some additional C++ test coverage for: alignas(int) struct S {}; to demonstrate we still suggest moving the keyword in that case.

clang/lib/Sema/SemaDecl.cpp

5296

I think this isn't quite right -- it'll return true in C++ for alignas: alignas(int) struct S {}; will give the wrong diagnostic now, I suspect. You need to check the language mode as well to ensure we're not in C++ mode.

5299–5304

5312–5317

Pretty sure you can use something like this instead now.

clang/test/C/drs/dr4xx.c

167

This diagnostic is incorrect but should be fixed with the suggested edit above.

clang/test/Parser/c1x-alignas.c

12–13

Same here.

[aaron.ballman]

@theo-lw -- are you planning to continue working on this review? (It's okay if you're not, I'm more wondering if I should remove you as the assignee on https://github.com/llvm/llvm-project/issues/58637)

[theo-lw]

In D141177#4592158, @aaron.ballman wrote:

@theo-lw -- are you planning to continue working on this review? (It's okay if you're not, I'm more wondering if I should remove you as the assignee on https://github.com/llvm/llvm-project/issues/58637)

Hey! Sorry, got busy with school. I can continue working on this.

[aaron.ballman]

In D141177#4592361, @theo-lw wrote:

In D141177#4592158, @aaron.ballman wrote:

@theo-lw -- are you planning to continue working on this review? (It's okay if you're not, I'm more wondering if I should remove you as the assignee on https://github.com/llvm/llvm-project/issues/58637)

Hey! Sorry, got busy with school. I can continue working on this.

No worries, it happens! Thank you for continuing work on this -- if you find you don't have the time, it's not a big deal, just let us know. :-)

[theo-lw]

Rebased on the main branch & addressed comments.

Added additional test case for C++ alignas.

[theo-lw]

Ran clang-format on this.

[aaron.ballman]

clang/lib/Sema/SemaDecl.cpp

5337–5346

I think this might be a bit more clear:

unsigned DiagID;
if (AL.isC11AlignasAttribute()) {
  // Don't use the message with placement with _Alignas.
  // This is because C doesnt let you use _Alignas on type declarations.
  DiagID = diag::warn_attribute_ignored;
} else if (AL.isRegularKeywordAttribute())
  DiagID = diag::err_declspec_keyword_has_no_effect;
else
  DiagID = diag::warn_declspec_attribute_ignored;

Diag(AL.getLoc(), DiagID) << AL << GetDiagnosticTypeSpecifierID(DS);

clang/test/Parser/c1x-alignas.c

12

I'd like to see a C23-specific test that does:

alignas(int) struct c7;

(the test doesn't have to be in this file, but it's to validate that we correctly handle alignas when it's a keyword that aliases to _Alignas.)

clang/lib/Sema/SemaDecl.cpp

if (DS.getTypeQualifiers() & DeclSpec::TQ_unaligned)
       Diag(DS.getUnalignedSpecLoc(), DiagID) << "__unaligned";
   }
 
   // Warn about ignored type attributes, for example:
   // __attribute__((aligned)) struct A;
   // Attributes should be placed after tag to apply to type declaration.
   if (!DS.getAttributes().empty() || !DeclAttrs.empty()) {
     DeclSpec::TST TypeSpecType = DS.getTypeSpecType();
+    auto WarnAttributeIgnored = [this, TypeSpecType](const ParsedAttr &AL) {
+      if (AL.isAlignasAttribute()) {
+        // Don't use the message with placement with _Alignas.
+        // This is because C doesnt let you use _Alignas on type declarations.
+        Diag(AL.getLoc(), diag::warn_attribute_ignored)
+            << GetDiagnosticTypeSpecifierID(TypeSpecType);
+      } else {
+        Diag(AL.getLoc(), diag::warn_declspec_attribute_ignored)
+            << AL << GetDiagnosticTypeSpecifierID(TypeSpecType);
+      }
+    };
+
     if (TypeSpecType == DeclSpec::TST_class ||
         TypeSpecType == DeclSpec::TST_struct ||
         TypeSpecType == DeclSpec::TST_interface ||
         TypeSpecType == DeclSpec::TST_union ||
         TypeSpecType == DeclSpec::TST_enum) {
-      for (const ParsedAttr &AL : DS.getAttributes())
-        Diag(AL.getLoc(), diag::warn_declspec_attribute_ignored)
-            << AL << GetDiagnosticTypeSpecifierID(TypeSpecType);
-      for (const ParsedAttr &AL : DeclAttrs)
-        Diag(AL.getLoc(), diag::warn_declspec_attribute_ignored)
-            << AL << GetDiagnosticTypeSpecifierID(TypeSpecType);
+      for (const ParsedAttr &AL : DS.getAttributes()) {
+        WarnAttributeIgnored(AL);
+      }
+      for (const ParsedAttr &AL : DeclAttrs) {
+        WarnAttributeIgnored(AL);
+      }
     }
   }
 
   return TagD;
 }
 
 /// We are trying to inject an anonymous member into the given scope;
 /// check if there's an existing declaration that can't be overloaded.
 ///
 /// \return true if this is a forbidden redeclaration
 static bool CheckAnonMemberRedeclaration(Sema &SemaRef,
                                          Scope *S,
                                          DeclContext *Owner,
                                          DeclarationName Name,
                                          SourceLocation NameLoc,
                                          bool IsUnion) {
   LookupResult R(SemaRef, Name, NameLoc, Sema::LookupMemberName,
                  Sema::ForVisibleRedeclaration);
   if (!SemaRef.LookupName(R, S)) return false;
 
   // Pick a representative declaration.
   NamedDecl *PrevDecl = R.getRepresentativeDecl()->getUnderlyingDecl();
   assert(PrevDecl && "Expected a non-null Decl");
 
   if (!SemaRef.isDeclInScope(PrevDecl, Owner, S))
     return false;
 
   SemaRef.Diag(NameLoc, diag::err_anonymous_record_member_redecl)
     << IsUnion << Name;
   SemaRef.Diag(PrevDecl->getLocation(), diag::note_previous_declaration);
 
   return true;
 }
 
 /// InjectAnonymousStructOrUnionMembers - Inject the members of the
 /// anonymous struct or union AnonRecord into the owning context Owner
 /// and scope S. This routine will be invoked just after we realize

clang/test/C/drs/dr4xx.c

void dr444(void) {
   _Alignas(int) int i;
    _Alignas(int) struct S {
     _Alignas(int) int i;
   } s;
 
   /* FIXME: This should be accepted as per this DR. */
   int j = (_Alignas(int) int){12}; /* expected-error {{expected expression}} */
 
- /* FIXME: The diagnostic in this case is really bad; moving the specifier to
-  * where the diagnostic recommends causes a different, more inscrutable error
-  * about anonymous structures.
-  */
-  _Alignas(int) struct T { /* expected-warning {{attribute '_Alignas' is ignored, place it after "struct" to apply attribute to type declaration}} */
+  _Alignas(int) struct T { /* expected-warning {{1 attribute ignored}} */
     int i;
   };
 
   struct U {
     _Alignas(int) int bit : 1; /* expected-error {{'_Alignas' attribute cannot be applied to a bit-field}} */
   };
 
   _Alignas(int) typedef int foo;  /* expected-error {{'_Alignas' attribute only applies to variables and fields}} */

clang/test/Parser/c1x-alignas.c

 // RUN: %clang_cc1 -std=c11 -fsyntax-only -verify %s
 // RUN: not %clang_cc1 -std=c99 -pedantic -fsyntax-only %s 2>&1 | FileCheck -check-prefix=CHECK-EXT %s
 
 _Alignas(4) char c1;
 unsigned _Alignas(long) char c2;
 char _Alignas(16) c3;
 
 char c4 _Alignas(32); // expected-error {{expected ';' after top level declarator}} expected-warning {{declaration does not declare anything}}
 
 char _Alignas(_Alignof(int)) c5;
 
+_Alignas(int) struct c6; // expected-warning {{1 attribute ignored}}
+
 // CHECK-EXT: '_Alignas' is a C11 extension
 // CHECK-EXT: '_Alignof' is a C11 extension