HomePhabricator

[NFC] Remove string parameter of annotation attribute from AST childs.

Authored by Tyker on Nov 9 2020, 6:06 AM.

Description

[NFC] Remove string parameter of annotation attribute from AST childs.
this simplifies using annotation attributes when using clang as library

Details

Committed
TykerNov 9 2020, 7:39 AM
Parents
rGd631e5240c9a: [testing] Add exhaustive ULT/UGT vector CTPOP to AArch64 and PPC
Branches
Unknown
Tags
Unknown

Event Timeline

Just an FYI, but this probably should have gotten a review as it's not NFC (it changes what information gets dumped in text and JSON form, impacts AST matching behavior, and other minor effects). That said, overall the direction seems fine to me, though I did question one of the changes.

/clang/include/clang/Basic/Attr.td
745

This seems odd to me -- this function is supposed to be used to generate an attribute that the user explicitly wrote, but it's defaulting the AttributeCommonInfo object to something that has no source range, etc. From some quick code inspection, it doesn't seem like this parameter needs to be defaulted (everyone calling AnnotateAttr::Create() is passing a range or the common info).

Tyker added a comment.Nov 11 2020, 7:54 AM

Just an FYI, but this probably should have gotten a review as it's not NFC (it changes what information gets dumped in text and JSON form, impacts AST matching behavior, and other minor effects). That said, overall the direction seems fine to me, though I did question one of the changes.

ok, my understanding was that NFC for normal compiler users, but yes this can make a difference for tools build on top of clang or AST dumping.

/clang/include/clang/Basic/Attr.td
745

this prototype is the same as one of the generated factory function prior to adding multiple arguments to annotate.

here is what was generate before D88645

static AnnotateAttr *CreateImplicit(ASTContext &Ctx, llvm::StringRef Annotation, const AttributeCommonInfo &CommonInfo = {SourceRange{}});
static AnnotateAttr *Create(ASTContext &Ctx, llvm::StringRef Annotation, const AttributeCommonInfo &CommonInfo = {SourceRange{}});
static AnnotateAttr *CreateImplicit(ASTContext &Ctx, llvm::StringRef Annotation, SourceRange Range, AttributeCommonInfo::Syntax Syntax);
static AnnotateAttr *Create(ASTContext &Ctx, llvm::StringRef Annotation, SourceRange Range, AttributeCommonInfo::Syntax Syntax);

Just an FYI, but this probably should have gotten a review as it's not NFC (it changes what information gets dumped in text and JSON form, impacts AST matching behavior, and other minor effects). That said, overall the direction seems fine to me, though I did question one of the changes.

ok, my understanding was that NFC for normal compiler users, but yes this can make a difference for tools build on top of clang or AST dumping.

Yeah, this is in one of those weird edge cases in some ways.

/clang/include/clang/Basic/Attr.td
745

Ohhh, that's certainly interesting! I wasn't aware there was a non-implicit overload that had a defaulted range. Thanks for pointing that out, I'll investigate the table generator to see if that can be improved. If I can, then I'll hit this change at the same time.

aaron.ballman added inline comments.Nov 12 2020, 10:12 AM
/clang/include/clang/Basic/Attr.td
745

I wound up making that improvement to the table generator and this code in b336826c1dd92adabef682f9b013b2e36bce066c.

psionic12 added inline comments.
/clang/include/clang/Basic/Attr.td
745

Just curious, what if I do want add arguments when using AnnotateAttr::Create? AnnotateAttr::Create with arguments would be really useful. Add all arguments in one string and parse them back is such an annoying job.

psionic12 added inline comments.Nov 12 2020, 7:55 PM
/clang/include/clang/Basic/Attr.td
745

Sorry I made a mistake, please ignore my comment above.