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);
----------------
simon_tatham wrote:
> 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.
I made this change, and discovered that as a side effect, this diagnostic is
now reported on the same one of the two source lines as all the others in my
test file – it's now reported against the attribute rather than the end of the
declaration it's applied to.
I guess that's extra evidence that you were right about where it belongs :-)
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
