aaron.ballman 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);
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67159/new/
https://reviews.llvm.org/D67159
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
