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:
> > > 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...)
> > OK – so actually what you meant to suggest was to put the diagnostic at
> > just some of the call sites for getBuiltinId?
>
> Yes! Sorry, I can see how I was unclear before. :-)
>
> > 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?
>
> Yes. `getBuiltinId()` already returns `0` in error cases without diagnosing,
> such as the function being unnamed or not being a builtin. I want to retain
> that property -- this function returns zero if the function is not a builtin.
> It's up to the caller of the function to decide whether a zero return value
> should be diagnosed or not.
>
> To be honest, this diagnostic feels like it belongs in SemaDeclAttr.cpp; it
> is placing a constraint on which declarations can have the attribute, so that
> should be checked *before* applying the attribute to the declaration. This
> also keeps the AST cleaner by not having an attribute on a function which
> should not be attributed.
> `getBuiltinId()` already returns `0` in error cases without diagnosing
Ah, I hadn't spotted that! That by itself makes it all make a lot more sense to
me.
> this diagnostic feels like it belongs in SemaDeclAttr.cpp
OK, I'll look at moving it there. Thanks for the pointer.
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
