aaron.ballman marked an inline comment as done.
aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:539
"packed attribute is unnecessary for %0">, InGroup<Packed>, DefaultIgnore;
+
}
----------------
Spurious newline?
================
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:
> 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 :-)
Fantastic to hear!
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4840-4841
+static void handleArmMveAliasAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+ if (!checkAttributeNumArgs(S, AL, 1))
+ return;
+
----------------
You don't need to do this, it should be handled automatically for you in
`handleCommonAttributeFeatures()`.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4843-4847
+ if (!AL.isArgIdent(0)) {
+ S.Diag(AL.getLoc(), diag::err_attribute_argument_n_type)
+ << AL << 1 << AANT_ArgumentIdentifier;
+ return;
+ }
----------------
I believe this is also automatically checked for you as well.
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
