[clang] [Clang] [Parser] Fixing all callers of `ParseExternalDeclaration` that didn't parse gnu attributes (PR #117148)

2024-11-25 Thread Mathys Gasnier via cfe-commits 


@@ -2314,10 +2314,12 @@ Parser::DeclGroupPtrTy 
Parser::ParseOpenMPDeclarativeDirectiveWithExtDecl(
   // Here we expect to see some function declaration.
   if (AS == AS_none) {
 assert(TagType == DeclSpec::TST_unspecified);
-ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
-MaybeParseCXX11Attributes(Attrs);
+ParsedAttributes DeclSpecAttrs(AttrFactory);

Mathys-Gasnier wrote:

Because `Attrs` is not a local variable but an argument passed to 
`ParseOpenMPDeclarativeDirectiveWithExtDecl`

https://github.com/llvm/llvm-project/pull/117148
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] [Parser] Fixing all callers of `ParseExternalDeclaration` that didn't parse gnu attributes (PR #117148)

2024-11-25 Thread Mathys Gasnier via cfe-commits 


@@ -480,10 +468,7 @@ Decl *Parser::ParseExportDeclaration() {
 
   while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
  Tok.isNot(tok::eof)) {
-ParsedAttributes DeclAttrs(AttrFactory);
-MaybeParseCXX11Attributes(DeclAttrs);

Mathys-Gasnier wrote:

Probably not, some place still use it, and we aren't able to replace it with 
the new function due to branching depending on different tokens. For example 
see ParseDeclCXX.cpp in `ParseLinkage`

https://github.com/llvm/llvm-project/pull/117148
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] [Parser] Fixing all callers of `ParseExternalDeclaration` that didn't parse gnu attributes (PR #117148)

2024-11-21 Thread Mathys Gasnier via cfe-commits 

https://github.com/Mathys-Gasnier created 
https://github.com/llvm/llvm-project/pull/117148

Not all callers of `ParseExternalDeclaration` were parsing gnu attributes, this 
caused issues with namespaces declarations attributes needing to be in a 
specific order (See #73890).
This PR fixes this in namespaces, exports, and three other places. It also adds 
a test to ensure a non-regression of this behavior.

Fixes: #73890 

>From 086b2e21da4febd31789fe717bae526af625627e Mon Sep 17 00:00:00 2001
From: Mathys-Gasnier 
Date: Thu, 21 Nov 2024 11:08:41 +0100
Subject: [PATCH] [Clang] [Parser] Fixing all callers of
 `ParseExternalDeclaration` that didn't parse gnu attributes

---
 clang/lib/Parse/ParseDeclCXX.cpp | 24 +++-
 clang/lib/Parse/ParseObjc.cpp|  8 +---
 clang/lib/Parse/ParseOpenMP.cpp  |  8 +---
 clang/lib/Parse/Parser.cpp   |  8 +---
 clang/test/Parser/cxx-attributes.cpp |  3 +++
 5 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index 010ac9c1a3e3a9..4f050d6632c356 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -267,9 +267,11 @@ void Parser::ParseInnerNamespace(const 
InnerNamespaceInfoList &InnerNSs,
 while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
Tok.isNot(tok::eof)) {
   ParsedAttributes DeclAttrs(AttrFactory);
-  MaybeParseCXX11Attributes(DeclAttrs);
-  ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
-  ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs);
+  ParsedAttributes DeclSpecAttrs(AttrFactory);
+  while (MaybeParseCXX11Attributes(DeclAttrs) ||
+  MaybeParseGNUAttributes(DeclSpecAttrs))
+;
+  ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs);
 }
 
 // The caller is what called check -- we are simply calling
@@ -468,9 +470,11 @@ Decl *Parser::ParseExportDeclaration() {
   if (Tok.isNot(tok::l_brace)) {
 // FIXME: Factor out a ParseExternalDeclarationWithAttrs.
 ParsedAttributes DeclAttrs(AttrFactory);
-MaybeParseCXX11Attributes(DeclAttrs);
-ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
-ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs);
+ParsedAttributes DeclSpecAttrs(AttrFactory);
+while (MaybeParseCXX11Attributes(DeclAttrs) ||
+MaybeParseGNUAttributes(DeclSpecAttrs))
+  ;
+ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs);
 return Actions.ActOnFinishExportDecl(getCurScope(), ExportDecl,
  SourceLocation());
   }
@@ -481,9 +485,11 @@ Decl *Parser::ParseExportDeclaration() {
   while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
  Tok.isNot(tok::eof)) {
 ParsedAttributes DeclAttrs(AttrFactory);
-MaybeParseCXX11Attributes(DeclAttrs);
-ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
-ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs);
+ParsedAttributes DeclSpecAttrs(AttrFactory);
+while (MaybeParseCXX11Attributes(DeclAttrs) ||
+MaybeParseGNUAttributes(DeclSpecAttrs))
+  ;
+ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs);
   }
 
   T.consumeClose();
diff --git a/clang/lib/Parse/ParseObjc.cpp b/clang/lib/Parse/ParseObjc.cpp
index bcbf4dfbabafa8..ec0572d58b3552 100644
--- a/clang/lib/Parse/ParseObjc.cpp
+++ b/clang/lib/Parse/ParseObjc.cpp
@@ -2287,10 +2287,12 @@ 
Parser::ParseObjCAtImplementationDeclaration(SourceLocation AtLoc,
 ObjCImplParsingDataRAII ObjCImplParsing(*this, ObjCImpDecl);
 while (!ObjCImplParsing.isFinished() && !isEofOrEom()) {
   ParsedAttributes DeclAttrs(AttrFactory);
-  MaybeParseCXX11Attributes(DeclAttrs);
-  ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
+  ParsedAttributes DeclSpecAttrs(AttrFactory);
+  while (MaybeParseCXX11Attributes(DeclAttrs) ||
+  MaybeParseGNUAttributes(DeclSpecAttrs))
+;
   if (DeclGroupPtrTy DGP =
-  ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs)) {
+  ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs)) {
 DeclGroupRef DG = DGP.get();
 DeclsInGroup.append(DG.begin(), DG.end());
   }
diff --git a/clang/lib/Parse/ParseOpenMP.cpp b/clang/lib/Parse/ParseOpenMP.cpp
index b91928063169ee..debfc7bd898864 100644
--- a/clang/lib/Parse/ParseOpenMP.cpp
+++ b/clang/lib/Parse/ParseOpenMP.cpp
@@ -2314,10 +2314,12 @@ Parser::DeclGroupPtrTy 
Parser::ParseOpenMPDeclarativeDirectiveWithExtDecl(
   // Here we expect to see some function declaration.
   if (AS == AS_none) {
 assert(TagType == DeclSpec::TST_unspecified);
-ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
-MaybeParseCXX11Attributes(Attrs);
+ParsedAttributes DeclSpecAttrs(AttrFactory);
+while (MaybeParseCXX11Attributes(Attrs) ||
+MaybeParseGNUAttributes(DeclSpecAttrs))
+

[clang] [Clang] [Parser] Fixing all callers of `ParseExternalDeclaration` that didn't parse gnu attributes (PR #117148)

2024-11-21 Thread Mathys Gasnier via cfe-commits 


@@ -267,9 +267,11 @@ void Parser::ParseInnerNamespace(const 
InnerNamespaceInfoList &InnerNSs,
 while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
Tok.isNot(tok::eof)) {
   ParsedAttributes DeclAttrs(AttrFactory);
-  MaybeParseCXX11Attributes(DeclAttrs);
-  ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
-  ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs);
+  ParsedAttributes DeclSpecAttrs(AttrFactory);
+  while (MaybeParseCXX11Attributes(DeclAttrs) ||
+  MaybeParseGNUAttributes(DeclSpecAttrs))
+;

Mathys-Gasnier wrote:

If we factor out `ParseExternalDeclaration` in a function that also parses the 
attributes, factoring out attributes parsing is probably not as usefull right ?

Since it's quite a lot of changes, should I do it in two commits ? One fixing 
the bug and adding missing attribute parsing, and the other factoring 
everything into a `ParseExternalDeclarationWithAttrs` and replacing all uses 
that fit the need.

https://github.com/llvm/llvm-project/pull/117148
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] [Parser] Fixing all callers of `ParseExternalDeclaration` that didn't parse gnu attributes (PR #117148)

2024-11-21 Thread Mathys Gasnier via cfe-commits 


@@ -267,9 +267,11 @@ void Parser::ParseInnerNamespace(const 
InnerNamespaceInfoList &InnerNSs,
 while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
Tok.isNot(tok::eof)) {
   ParsedAttributes DeclAttrs(AttrFactory);
-  MaybeParseCXX11Attributes(DeclAttrs);
-  ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
-  ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs);
+  ParsedAttributes DeclSpecAttrs(AttrFactory);
+  while (MaybeParseCXX11Attributes(DeclAttrs) ||
+  MaybeParseGNUAttributes(DeclSpecAttrs))
+;

Mathys-Gasnier wrote:

Just pushed a new commit with what we talked about.

I'm not a big fan about the comment on the function I pushed, but I cannot find 
a wording that please me, I'm open to suggestion.

https://github.com/llvm/llvm-project/pull/117148
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] [Parser] Fixing all callers of `ParseExternalDeclaration` that didn't parse gnu attributes (PR #117148)

2024-11-21 Thread Mathys Gasnier via cfe-commits 

https://github.com/Mathys-Gasnier updated 
https://github.com/llvm/llvm-project/pull/117148

>From 086b2e21da4febd31789fe717bae526af625627e Mon Sep 17 00:00:00 2001
From: Mathys-Gasnier 
Date: Thu, 21 Nov 2024 11:08:41 +0100
Subject: [PATCH 1/2] [Clang] [Parser] Fixing all callers of
 `ParseExternalDeclaration` that didn't parse gnu attributes

---
 clang/lib/Parse/ParseDeclCXX.cpp | 24 +++-
 clang/lib/Parse/ParseObjc.cpp|  8 +---
 clang/lib/Parse/ParseOpenMP.cpp  |  8 +---
 clang/lib/Parse/Parser.cpp   |  8 +---
 clang/test/Parser/cxx-attributes.cpp |  3 +++
 5 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index 010ac9c1a3e3a9..4f050d6632c356 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -267,9 +267,11 @@ void Parser::ParseInnerNamespace(const 
InnerNamespaceInfoList &InnerNSs,
 while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
Tok.isNot(tok::eof)) {
   ParsedAttributes DeclAttrs(AttrFactory);
-  MaybeParseCXX11Attributes(DeclAttrs);
-  ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
-  ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs);
+  ParsedAttributes DeclSpecAttrs(AttrFactory);
+  while (MaybeParseCXX11Attributes(DeclAttrs) ||
+  MaybeParseGNUAttributes(DeclSpecAttrs))
+;
+  ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs);
 }
 
 // The caller is what called check -- we are simply calling
@@ -468,9 +470,11 @@ Decl *Parser::ParseExportDeclaration() {
   if (Tok.isNot(tok::l_brace)) {
 // FIXME: Factor out a ParseExternalDeclarationWithAttrs.
 ParsedAttributes DeclAttrs(AttrFactory);
-MaybeParseCXX11Attributes(DeclAttrs);
-ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
-ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs);
+ParsedAttributes DeclSpecAttrs(AttrFactory);
+while (MaybeParseCXX11Attributes(DeclAttrs) ||
+MaybeParseGNUAttributes(DeclSpecAttrs))
+  ;
+ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs);
 return Actions.ActOnFinishExportDecl(getCurScope(), ExportDecl,
  SourceLocation());
   }
@@ -481,9 +485,11 @@ Decl *Parser::ParseExportDeclaration() {
   while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
  Tok.isNot(tok::eof)) {
 ParsedAttributes DeclAttrs(AttrFactory);
-MaybeParseCXX11Attributes(DeclAttrs);
-ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
-ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs);
+ParsedAttributes DeclSpecAttrs(AttrFactory);
+while (MaybeParseCXX11Attributes(DeclAttrs) ||
+MaybeParseGNUAttributes(DeclSpecAttrs))
+  ;
+ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs);
   }
 
   T.consumeClose();
diff --git a/clang/lib/Parse/ParseObjc.cpp b/clang/lib/Parse/ParseObjc.cpp
index bcbf4dfbabafa8..ec0572d58b3552 100644
--- a/clang/lib/Parse/ParseObjc.cpp
+++ b/clang/lib/Parse/ParseObjc.cpp
@@ -2287,10 +2287,12 @@ 
Parser::ParseObjCAtImplementationDeclaration(SourceLocation AtLoc,
 ObjCImplParsingDataRAII ObjCImplParsing(*this, ObjCImpDecl);
 while (!ObjCImplParsing.isFinished() && !isEofOrEom()) {
   ParsedAttributes DeclAttrs(AttrFactory);
-  MaybeParseCXX11Attributes(DeclAttrs);
-  ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
+  ParsedAttributes DeclSpecAttrs(AttrFactory);
+  while (MaybeParseCXX11Attributes(DeclAttrs) ||
+  MaybeParseGNUAttributes(DeclSpecAttrs))
+;
   if (DeclGroupPtrTy DGP =
-  ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs)) {
+  ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs)) {
 DeclGroupRef DG = DGP.get();
 DeclsInGroup.append(DG.begin(), DG.end());
   }
diff --git a/clang/lib/Parse/ParseOpenMP.cpp b/clang/lib/Parse/ParseOpenMP.cpp
index b91928063169ee..debfc7bd898864 100644
--- a/clang/lib/Parse/ParseOpenMP.cpp
+++ b/clang/lib/Parse/ParseOpenMP.cpp
@@ -2314,10 +2314,12 @@ Parser::DeclGroupPtrTy 
Parser::ParseOpenMPDeclarativeDirectiveWithExtDecl(
   // Here we expect to see some function declaration.
   if (AS == AS_none) {
 assert(TagType == DeclSpec::TST_unspecified);
-ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
-MaybeParseCXX11Attributes(Attrs);
+ParsedAttributes DeclSpecAttrs(AttrFactory);
+while (MaybeParseCXX11Attributes(Attrs) ||
+MaybeParseGNUAttributes(DeclSpecAttrs))
+  ;
 ParsingDeclSpec PDS(*this);
-Ptr = ParseExternalDeclaration(Attrs, EmptyDeclSpecAttrs, &PDS);
+Ptr = ParseExternalDeclaration(Attrs, DeclSpecAttrs, &PDS);
   } else {
 Ptr =
 ParseCXXClassMemberDeclarationWithPragmas(AS, Attrs, TagType, Tag);
diff --git a/clang/lib/Parse/Parse

[clang] [Clang] [Parser] Fixing all callers of `ParseExternalDeclaration` that didn't parse gnu attributes (PR #117148)

2024-11-21 Thread Mathys Gasnier via cfe-commits 


@@ -267,9 +267,11 @@ void Parser::ParseInnerNamespace(const 
InnerNamespaceInfoList &InnerNSs,
 while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
Tok.isNot(tok::eof)) {
   ParsedAttributes DeclAttrs(AttrFactory);
-  MaybeParseCXX11Attributes(DeclAttrs);
-  ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
-  ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs);
+  ParsedAttributes DeclSpecAttrs(AttrFactory);
+  while (MaybeParseCXX11Attributes(DeclAttrs) ||
+  MaybeParseGNUAttributes(DeclSpecAttrs))
+;

Mathys-Gasnier wrote:

Yes, but it could be understood the wrong way around I think...
Let's wait on feedback from others, maybe someone will have an eureka moment.

https://github.com/llvm/llvm-project/pull/117148
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits