[PATCH] D67159: [clang] New __attribute__((__clang_arm_mve_alias)).

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham marked an inline comment as done.
simon_tatham added inline comments.


================
Comment at: clang/lib/AST/Decl.cpp:3107
+    if (!ArmMveAliasValid(BuiltinID, getIdentifier()->getName())) {
+      getASTContext().getDiagnostics().Report(
+        getLocation(), diag::err_attribute_arm_mve_alias);
----------------
aaron.ballman wrote:
> 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.
OK – so actually what you meant to suggest was to put the diagnostic at just //some// of the call sites for `getBuiltinId`?

With the intended behavior being that the Sema test in this patch should still provoke all the expected diagnostics in an ordinary compilation context, but in other situations like AST matchers, it would be better for `getBuiltinId` to //silently// returns 0 if there's an illegal ArmMveAlias attribute?

(I'm just checking I've understood you correctly before I do the work...)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67159/new/

https://reviews.llvm.org/D67159