pmatos marked 4 inline comments as done.
pmatos added a comment.
Thanks @aaron.ballman . I think I have now address all outstanding comments.
================
Comment at: clang/include/clang/Basic/Attr.td:4129
+ let Documentation = [WebAssemblyExportNameDocs];
+ let Subjects = SubjectList<[TypedefName], ErrorDiag>;
+}
----------------
aaron.ballman wrote:
> pmatos wrote:
> > aaron.ballman wrote:
> > > erichkeane wrote:
> > > > pmatos wrote:
> > > > > erichkeane wrote:
> > > > > > pmatos wrote:
> > > > > > > erichkeane wrote:
> > > > > > > > Note that it seems this is likely not going to work correctly
> > > > > > > > on template type aliases! You probably want to make sure that
> > > > > > > > is acceptable to you.
> > > > > > > Documentation says about Subject:
> > > > > > >
> > > > > > > ```
> > > > > > > // The things to which an attribute can appertain
> > > > > > > SubjectList Subjects;
> > > > > > > ```
> > > > > > >
> > > > > > > Given this can be attached to function pointer types, is the
> > > > > > > subject then just Type ?
> > > > > > IIRC, the list of subjects are the Decls and Stmt types that are
> > > > > > possibly allowed. I don't thnk it would be just 'Type' there.
> > > > > >
> > > > > > As you have it, it is only allowed on TypedefNameDecl.
> > > > > >
> > > > > > See the SubsetSubject's at the top of this file for some examples
> > > > > > on how you could constrain a special matcher to only work on
> > > > > > function pointer decls.
> > > > > Good point @erichkeane . What do you think of FunctionPointer
> > > > > implementation here?
> > > > That seems acceptable to me.
> > > That makes sense to me as well, but FWIW, type attributes don't currently
> > > have much of any tablegen automation (unlike decl and stmt attributes),
> > > so the subject list isn't strictly necessary here. But it's still
> > > appreciated because 1) it's documentation, and 2) we want to tablegen
> > > more and this helps us with that goal.
> > @erichkeane Yes, you're right. Something like
> >
> > ```
> > using IntIntFuncref = int(*)(int) __funcref;
> > ```
> >
> > doesn't work. Why is this not accepted? Is it due to the Subjects listed?
> Not due to the subjects -- keyword attributes require manual parsing and type
> attributes require manual processing (we don't tablegen as much as I'd like
> for type attributes). But that code should be accepted (you should add a test
> case for it as well).
Seems to be fine now. Test added.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:6930
+def WebAssemblyFuncrefDocs : Documentation {
+ let Category = DocCatFunction;
+ let Content = [{
----------------
aaron.ballman wrote:
> `DocCatType`, right?
Yes.
================
Comment at: clang/test/Sema/wasm-refs.c:50
+typedef void (*__funcref funcref_t)();
+typedef void (*__funcref __funcref funcref_fail_t)(); // expected-warning
{{attribute '__funcref' is already applied}}
----------------
aaron.ballman wrote:
> Should we also test two-level application of the qualifier, as in?
> ```
> using uh_oh = funcref_t __funcref;
> ```
Double qualification shouldn't be an issue - adding this to
`SemaCXX/wasm-funcref.cpp`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128440/new/
https://reviews.llvm.org/D128440
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
