aaron.ballman marked 2 inline comments as done.
aaron.ballman added inline comments.
================
Comment at: clang/lib/AST/Decl.cpp:3082
+static bool ArmMveAliasValid(unsigned BuiltinID, StringRef AliasName) {
+ // This will be filled in by Tablegen which isn't written yet
+ return false;
----------------
simon_tatham wrote:
> aaron.ballman wrote:
> > Comment should have a `FIXME` prefix, but tbh, I'm a little uncomfortable
> > adopting the attribute without this being implemented. I assume the plan is
> > to tablegen a header file that gets included here to provide the lookup?
> Yes: D67161, which I intend to commit as part of the same patch series once
> everything in it is approved, fills in this function with tablegen output.
>
> I could roll both into the same monolithic patch, but I thought it would be
> better to break it up into manageable pieces as far as possible, especially
> when some of them (like this one) would need to be reviewed by people with
> significantly different expertise from the rest.
Ah, I didn't realize that was so involved; keeping them split makes sense.
================
Comment at: clang/lib/AST/Decl.cpp:3107
+ if (!ArmMveAliasValid(BuiltinID, getIdentifier()->getName())) {
+ getASTContext().getDiagnostics().Report(
+ getLocation(), diag::err_attribute_arm_mve_alias);
----------------
simon_tatham wrote:
> aaron.ballman wrote:
> > I'm not certain how comfortable I am with having this function produce a
> > diagnostic. That seems like unexpected behavior for a function attempting
> > to get a builtin ID. I think this should be the responsibility of the
> > caller.
> The //caller//? But there are many possible callers of this function. You
> surely didn't mean to suggest duplicating the diagnostic at all those call
> sites.
>
> Perhaps it would make more sense to have all the calculation in this
> `getBuiltinID` method move into a function called once, early in the
> `FunctionDecl`'s lifetime, which figures out the builtin ID (if any) and
> stashes it in a member variable? Then //that// would issue the diagnostic, if
> any (and it would be called from a context where that was a sensible thing to
> do), and `getBuiltinID` itself would become a mere accessor function.
> The caller? But there are many possible callers of this function. You surely
> didn't mean to suggest duplicating the diagnostic at all those call sites.
Yes, I did. :-) No caller is going to expect that calling a `const` function
that gets a builtin ID is going to issue diagnostics and so this runs the risk
of generating diagnostics in surprising situations, such as from AST matchers.
> Perhaps it would make more sense to have all the calculation in this
> getBuiltinID method move into a function called once, early in the
> FunctionDecl's lifetime, which figures out the builtin ID (if any) and
> stashes it in a member variable? Then that would issue the diagnostic, if any
> (and it would be called from a context where that was a sensible thing to
> do), and getBuiltinID itself would become a mere accessor function.
That might make sense, but I don't have a good idea of what performance
concerns that might raise. If there are a lot of functions and we never need to
check if they have a builtin ID, that could be expensive for little gain.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7442
+ case ParsedAttr::AT_ClangBuiltinOverride:
+ handleClangBuiltinOverrideAttribute(S, D, AL);
+ break;
----------------
simon_tatham wrote:
> aaron.ballman wrote:
> > You should be able to call
> > `handleSimpleAttribute<ClangBuiltinOverrideAttr>()` instead.
> Oh, nearly forgot: apparently I can't, because that template expects the
> attribute to have a constructor that takes only an `ASTContext` and an
> `AttributeCommonInfo`. But the constructor for my attribute also takes an
> `IdentifierInfo` giving the builtin name.
Ah, good point.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67159/new/
https://reviews.llvm.org/D67159
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
