rsmith added a comment.
I think this patch is nearly ready to commit. However... if you want to handle
macros that specify a sequence of attributes, we should switch to treating the
macro name as a separate type sugar node rather than modeling it as a name for
the `AttributedType` node. Up to you how we proceed from here -- if you want to
land this patch with a one-attribute-in-the-macro restriction, that seems fine;
if you'd prefer to go straight to a more general approach where we add a new
type sugar representation for a single macro describing (potentially) multiple
attributes, that's fine too.
================
Comment at: lib/AST/ASTDiagnostic.cpp:78
+ AttributedType::getNullabilityAttrKind(*nullability), RT, RT,
+
FT->getReturnType()->getAs<AttributedType>()->getMacroIdentifier());
}
----------------
Use `castAs` here and below (because it would be a programming error if the
type isn't an `AttributedType`).
================
Comment at: lib/AST/ASTImporter.cpp:991
+ T->getAttrKind(), ToModifiedType, ToEquivalentType,
+ T->getMacroIdentifier());
}
----------------
You need to import the identifier; the source and destination ASTs have
different identifier tables. (`Importer.Import(T->getMacroIdentifier())`).
================
Comment at: lib/AST/TypePrinter.cpp:1370
+
+ // Remove the underlying address space so it won't be printed.
+ SplitQualType SplitTy = T->getModifiedType().split();
----------------
leonardchan wrote:
> rsmith wrote:
> > leonardchan wrote:
> > > rsmith wrote:
> > > > leonardchan wrote:
> > > > > rsmith wrote:
> > > > > > This is unnecessary; just print the modified type here. (The
> > > > > > modified type by definition does not have the attribute applied to
> > > > > > it.)
> > > > > When you say the modified type, do you mean just the type without
> > > > > it's qualifiers? I wasn't sure if removing all the qualifiers would
> > > > > suffice since there were also other non-address_space qualifiers
> > > > > that could be printed.
> > > > I mean `T->getModifiedType()`, which tracks what the type was before
> > > > the attribute was applied.
> > > Oh, I understand that you meant `T->getModifiedType()`. This is a special
> > > case when printing the `address_space` since even though the attribute is
> > > applied and printed here, when we reach the qualifiers of the modified
> > > type, the address space will still be printed unless we remove it here.
> > >
> > > I'm not sure if there's a better way to do this though.
> > If the address space qualifiers are present on the modified type, then
> > that's a bug in the creation of the `AttributedType`. And indeed looking at
> > r340765, I see we are passing the wrong type in as the modified type when
> > creating the `AttributedType`. Please fix that, and then just use
> > `getModifiedType` here.
> Making another patch for this.
Incidentally, the last two arguments to the `AddressSpaceAttr` constructor in
that patch (address space and spelling list index) are also reversed.
================
Comment at: lib/Parse/ParseDecl.cpp:138
while (Tok.is(tok::kw___attribute)) {
+ Token AttrTok = Tok;
+ unsigned OldNumAttrs = attrs.size();
----------------
No need to copy the entire `Token` here, you only use its location before. It's
also idiomatic to fold this into consuming the token:
`SourceLocation AttrTokLoc = ConsumeToken();`
================
Comment at: lib/Parse/ParseDecl.cpp:215-220
+ bool AttrStartIsInMacro =
+ (AttrTokLoc.isMacroID() && Lexer::isAtStartOfMacroExpansion(
+ AttrTokLoc, SrcMgr, PP.getLangOpts()));
+ bool AttrEndIsInMacro =
+ (Loc.isMacroID() &&
+ Lexer::isAtEndOfMacroExpansion(Loc, SrcMgr, PP.getLangOpts()));
----------------
You're only looking at the innermost macro expansion; I still think we should
take the outermost one. (You can iteratively walk the expansion locations of
the start and end locations (`SourceManager::getExpansionLocStart`/`End`) to
build a list of `FileID`s representing macro expansions that each of them is
included in, and then take the last common such `FileID` that represents an
object-like macro. You can use `SourceManager::getSLocEntry` to get the
corresponding `ExpansionInfo` for the macro, `isFunctionMacroExpansion` to
determine if it's a function-like or object-like macro, and once you know it's
object-like, take the spelling locations of the `ExpansionLocStart` and
`ExpansionLocEnd` to find the name of the macro.
================
Comment at: lib/Parse/ParseDecl.cpp:244-252
+ // If this was declared in a macro, attatch the macro IdentifierInfo to the
+ // parsed attribute.
+ for (const auto &MacroPair : PP.getAttributeMacros()) {
+ if (SourceLocInSourceRange(AttrTok.getLocation(), MacroPair.first,
+ PP.getSourceManager())) {
+ ApplyMacroIIToParsedAttrs(attrs, NumParsedAttrs, MacroPair.second);
+ break;
----------------
leonardchan wrote:
> rsmith wrote:
> > You shouldn't do this if `NumParsedAttrs != 1`. We're only looking for a
> > macro that exactly covers one attribute.
> >
> > (Also, your computation of the number of attributes in the attribute list
> > is not correct in the presence of late-parsed attributes.)
> One of the things we would like is for this to cover more than one attribute
> in the attribute list since in sparse, `address_space` is sometimes used with
> `noderef`.
>
> So given `# define __user __attribute__((noderef, address_space(1)))`,
> `__user` would be saved into the `ParsedAttr` made for `noderef` and
> `address_space`.
>
> What would be the appropriate way to track newly added attributes into the
> `ParsedAttributes`, including late-parsed attributes?
> One of the things we would like is for this to cover more than one attribute
> in the attribute list since in sparse, `address_space` is sometimes used with
> `noderef`.
Hold on, this is a new requirement compared to what we'd previously discussed
(giving a name to an address space). How important is this use case to you?
I don't think it's a reasonable AST model to assign a macro identifier to an
`AttributedType` if the macro defines multiple attributes. If you really want
to handle that use case, I think that an identifier on the `AttributedType` is
the wrong way to model it, and we should instead be creating a new type sugar
node representing a type decoration written as a macro.
Assuming you want to go ahead with the current patch direction (at least in the
short term), please add the "only one attribute in the macro" check.
Repository:
rC Clang
https://reviews.llvm.org/D51329
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
