[PATCH] D128440: [WebAssembly] Initial support for reference type funcref in clang

[Paulo Matos via Phabricator via cfe-commits]

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