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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
