aaron.ballman added inline comments.
Herald added a subscriber: the_o.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:5280
+ // Check the attribute arguments.
+ if (AL.getNumArgs() > 1) {
+ S.Diag(AL.getLoc(), diag::err_attribute_too_many_arguments)
----------------
apazos wrote:
> aaron.ballman wrote:
> > Please call `checkAttributeNumArgs()` instead; the error you're using is
> > incorrect (it's used for variadic parameters where you receive more
> > arguments than you expect).
> The argument is optional and at most one argument , I will use
> checkAttributeAtMostNumArgs instead
Ah, yeah, I didn't pick up the optional part when writing my comment; this LG.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:5301
+
+ if (!isFunctionOrMethod(D)) {
+ S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
----------------
apazos wrote:
> aaron.ballman wrote:
> > I don't think you need to perform this check -- I believe it's handled
> > automatically (because you don't have custom parsing enabled).
> I think need it. Will double check in the test.
See `handleCommonAttributeFeatures()` -- it calls `diagnoseAppertainsTo()`
which handles this for you. As it is, your check here does not match the
subject list on the attribute. The declarative bit says it only appertains to a
function and this check is for a function or Obj-C method.
Which brings up my next question: should this appertain to an ObjC method?
================
Comment at: test/Sema/riscv-interrupt-attr.c:18
+
+__attribute__((interrupt("user"), interrupt("supervisor"))) void foo6() { } //
expected-warning {{repeated RISC-V 'interrupt' attribute}} \
+ //
expected-note {{repeated RISC-V 'interrupt' attribute is here}}
----------------
apazos wrote:
> aaron.ballman wrote:
> > You should also add tests for:
> > ```
> > __attribute__((interrupt("user"))) void f(void);
> > __attribute__((interrupt("machine"))) void f(void);
> >
> > void f(void) { }
> >
> > [[gnu::interrupt("user") gnu::interrupt("machine")]] void g() {}
> >
> > [[gnu::interrupt("user")]] [[gnu::interrupt("machine")]] void h() {}
> > ```
> For this test case tt seems LLVM honors the last setting, "machine".
> But gcc is honoring the first.
> I think the last setting should prevail. Will check with GCC folks.
Do all of these cases get diagnosed as being a repeated interrupt attribute?
Should add them as test cases.
https://reviews.llvm.org/D48412
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
