[Parser][NFC] Move the core parsing of an attribute into a separate method #107300
Conversation
…ethod Refactor attribute parsing so that the main code parsing an attribute can be called by a separate code path that doesn't start with the '__attribute' keyword.
bwendling
requested review from
cor3ntin and
alexey-bataev
and removed request for
cor3ntin
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Bill Wendling (bwendling) ChangesRefactor attribute parsing so that the main code parsing an attribute can be called by a separate code path that doesn't start with the '__attribute' keyword. Full diff: https://github.com/llvm/llvm-project/pull/107300.diff 2 Files Affected:
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index a7513069ff5da0..895f0436075356 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -2943,6 +2943,10 @@ class Parser : public CodeCompletionHandler {
return false;
}
+ bool ParseGNUSingleAttribute(ParsedAttributes &Attrs,
+ SourceLocation &EndLoc,
+ LateParsedAttrList *LateAttrs = nullptr,
+ Declarator *D = nullptr);
void ParseGNUAttributes(ParsedAttributes &Attrs,
LateParsedAttrList *LateAttrs = nullptr,
Declarator *D = nullptr);
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 78d729c5ef7d8a..3ddc37db3a1bd1 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -146,6 +146,72 @@ void Parser::ParseAttributes(unsigned WhichAttrKinds, ParsedAttributes &Attrs,
} while (MoreToParse);
}
+bool Parser::ParseGNUSingleAttribute(ParsedAttributes &Attrs,
+ SourceLocation &EndLoc,
+ LateParsedAttrList *LateAttrs,
+ Declarator *D) {
+ IdentifierInfo *AttrName = Tok.getIdentifierInfo();
+ if (!AttrName)
+ return true;
+
+ SourceLocation AttrNameLoc = ConsumeToken();
+
+ if (Tok.isNot(tok::l_paren)) {
+ Attrs.addNew(AttrName, AttrNameLoc, nullptr, AttrNameLoc, nullptr, 0,
+ ParsedAttr::Form::GNU());
+ return false;
+ }
+
+ bool LateParse = false;
+ if (!LateAttrs)
+ LateParse = false;
+ else if (LateAttrs->lateAttrParseExperimentalExtOnly()) {
+ // The caller requested that this attribute **only** be late
+ // parsed for `LateAttrParseExperimentalExt` attributes. This will
+ // only be late parsed if the experimental language option is enabled.
+ LateParse = getLangOpts().ExperimentalLateParseAttributes &&
+ IsAttributeLateParsedExperimentalExt(*AttrName);
+ } else {
+ // The caller did not restrict late parsing to only
+ // `LateAttrParseExperimentalExt` attributes so late parse
+ // both `LateAttrParseStandard` and `LateAttrParseExperimentalExt`
+ // attributes.
+ LateParse = IsAttributeLateParsedExperimentalExt(*AttrName) ||
+ IsAttributeLateParsedStandard(*AttrName);
+ }
+
+ // Handle "parameterized" attributes
+ if (!LateParse) {
+ ParseGNUAttributeArgs(AttrName, AttrNameLoc, Attrs, &EndLoc, nullptr,
+ SourceLocation(), ParsedAttr::Form::GNU(), D);
+ return false;
+ }
+
+ // Handle attributes with arguments that require late parsing.
+ LateParsedAttribute *LA =
+ new LateParsedAttribute(this, *AttrName, AttrNameLoc);
+ LateAttrs->push_back(LA);
+
+ // Attributes in a class are parsed at the end of the class, along
+ // with other late-parsed declarations.
+ if (!ClassStack.empty() && !LateAttrs->parseSoon())
+ getCurrentClass().LateParsedDeclarations.push_back(LA);
+
+ // Be sure ConsumeAndStoreUntil doesn't see the start l_paren, since it
+ // recursively consumes balanced parens.
+ LA->Toks.push_back(Tok);
+ ConsumeParen();
+ // Consume everything up to and including the matching right parens.
+ ConsumeAndStoreUntil(tok::r_paren, LA->Toks, /*StopAtSemi=*/true);
+
+ Token Eof;
+ Eof.startToken();
+ Eof.setLocation(Tok.getLocation());
+ LA->Toks.push_back(Eof);
+
+ return false;
+}
+
/// ParseGNUAttributes - Parse a non-empty attributes list.
///
/// [GNU] attributes:
@@ -223,64 +289,9 @@ void Parser::ParseGNUAttributes(ParsedAttributes &Attrs,
AttributeCommonInfo::Syntax::AS_GNU);
break;
}
- IdentifierInfo *AttrName = Tok.getIdentifierInfo();
- if (!AttrName)
- break;
-
- SourceLocation AttrNameLoc = ConsumeToken();
-
- if (Tok.isNot(tok::l_paren)) {
- Attrs.addNew(AttrName, AttrNameLoc, nullptr, AttrNameLoc, nullptr, 0,
- ParsedAttr::Form::GNU());
- continue;
- }
-
- bool LateParse = false;
- if (!LateAttrs)
- LateParse = false;
- else if (LateAttrs->lateAttrParseExperimentalExtOnly()) {
- // The caller requested that this attribute **only** be late
- // parsed for `LateAttrParseExperimentalExt` attributes. This will
- // only be late parsed if the experimental language option is enabled.
- LateParse = getLangOpts().ExperimentalLateParseAttributes &&
- IsAttributeLateParsedExperimentalExt(*AttrName);
- } else {
- // The caller did not restrict late parsing to only
- // `LateAttrParseExperimentalExt` attributes so late parse
- // both `LateAttrParseStandard` and `LateAttrParseExperimentalExt`
- // attributes.
- LateParse = IsAttributeLateParsedExperimentalExt(*AttrName) ||
- IsAttributeLateParsedStandard(*AttrName);
- }
-
- // Handle "parameterized" attributes
- if (!LateParse) {
- ParseGNUAttributeArgs(AttrName, AttrNameLoc, Attrs, &EndLoc, nullptr,
- SourceLocation(), ParsedAttr::Form::GNU(), D);
- continue;
- }
- // Handle attributes with arguments that require late parsing.
- LateParsedAttribute *LA =
- new LateParsedAttribute(this, *AttrName, AttrNameLoc);
- LateAttrs->push_back(LA);
-
- // Attributes in a class are parsed at the end of the class, along
- // with other late-parsed declarations.
- if (!ClassStack.empty() && !LateAttrs->parseSoon())
- getCurrentClass().LateParsedDeclarations.push_back(LA);
-
- // Be sure ConsumeAndStoreUntil doesn't see the start l_paren, since it
- // recursively consumes balanced parens.
- LA->Toks.push_back(Tok);
- ConsumeParen();
- // Consume everything up to and including the matching right parens.
- ConsumeAndStoreUntil(tok::r_paren, LA->Toks, /*StopAtSemi=*/true);
-
- Token Eof;
- Eof.startToken();
- Eof.setLocation(Tok.getLocation());
- LA->Toks.push_back(Eof);
+ if (ParseGNUSingleAttribute(Attrs, EndLoc, LateAttrs, D))
+ break;
} while (Tok.is(tok::comma));
if (ExpectAndConsume(tok::r_paren))
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
clang/include/clang/Parse/Parser.h
Outdated
| @@ -2943,6 +2943,9 @@ class Parser : public CodeCompletionHandler { | |||
| return false; | |||
| } | |||
|
|
|||
| bool ParseGNUSingleAttribute(ParsedAttributes &Attrs, SourceLocation &EndLoc, | |||
There was a problem hiding this comment.
This could be read as parsing __attribute__((foo)) as opposed to just foo alone. However, I'm not certain I have a better name. ParseSingleGNUAttributeAfterIntroducer was the best I could come up with.
If that doesn't make anyone happy, can we switch to ParseSingleGNUAttribute() instead?
There was a problem hiding this comment.
I don't have a preference for any of the 3 names, but a comment explaining WHERE it is in the source would be very helpful.
That said, what makes this GNU specific? Once we're inside of the braces/bracket/etc, I'd expect there to be only slightly minor differences between the three attribute parsings, so I'd expect them to be a single function with a few special cases (std attributes with an optional namespace, etc).
There was a problem hiding this comment.
If that doesn't make anyone happy, can we switch to ParseSingleGNUAttribute() instead?
I think this is my preferred name out of the three.
That said, what makes this GNU specific? Once we're inside of the braces/bracket/etc, I'd expect there to be only slightly minor differences between the three attribute parsings, so I'd expect them to be a single function with a few special cases (std attributes with an optional namespace, etc).
My main purpose for this refactor is to help aid parsing __builtin_has_attribute() (WIP), where the second argument is an attribute. I don't think I'll need to handle non-GNU attributes for that (though I may be wrong, I'll have to check GCC's implementation).
There was a problem hiding this comment.
Ah, I see. The WG14 specified one is __builtin_has_c_attribute, correct?
That said, I'd think that we would want the Clang behavior to answer the right thing for ALL of our attributes (including the MSVC ones). But I then wonder if, since this is a GCC originating builtin, if we should only accept ones with a GCC spelling.
So I guess I'm on the fence here. I REALLY wish the name was more GCC specific, that way we could just match their behavior, but with the name as it is, I think we have to answer that question for ANY attribute, including declspecs, standard attributes, or clang extensions.
So perhaps that changes this refactor?
There was a problem hiding this comment.
Ah, I see. The WG14 specified one is __builtin_has_c_attribute, correct?
No clue. I haven't heard of that builtin before. :-)
So perhaps that changes this refactor?
I'm not sure how? Let's table discussion about __builtin_has_attribute until I get a PR. :-) The GNU documentation for that builtin indicates that it supports only GNU-style attributes. If we need to support more for some reason, then we can refactor other places.
There was a problem hiding this comment.
I'm not sure how?
It would change the purpose of this patch. If your goal is ot extract this for use in the builtin, and the builtin will work with all attribute names, than this isn't going to be a useful refactor.
The GNU documentation for that builtin indicates that it supports only GNU-style attributes. If we need to support more for some reason, then we can refactor other places.
My point is: We constantly and consistently 'generalize' the GNU builtins, the GNU documentation, which documents a product that only supports GNU style attributes, claims it only supports GNU style attributes is a bit of a tautology.
If we take it for clang, we need to generalize it and 'make it work' for all attributes. BUT I am ok discussing that in the next patch.
Refactor attribute parsing so that the main code parsing an attribute can be called by a separate code path that doesn't start with the '__attribute' keyword.