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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added a comment.

Generally LGTM with a few comments sprinkled around.



================
Comment at: clang/include/clang/Basic/Attr.td:4129
+  let Documentation = [WebAssemblyExportNameDocs];
+  let Subjects = SubjectList<[TypedefName], ErrorDiag>;
+}
----------------
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).


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6930
+def WebAssemblyFuncrefDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
----------------
`DocCatType`, right?


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6933
+Clang supports the ``__funcref`` attribute for the WebAssembly target. 
+This attribute may be attached to a function pointer declaration, where it modifies 
+its underlying representation to be a WebAssembly ``funcref``.
----------------
"Function pointer declaration" reads a bit odd to me, I think you mean "function pointer type", right?

e.g.,
`void (*)() __funcref` is fine, as is `void (*fp)() __funcref` -- the attribute appertains to the type, not to the declaration?


================
Comment at: clang/lib/Sema/SemaType.cpp:7350
+static bool HandleWebAssemblyFuncrefAttr(TypeProcessingState &State,
+                                         QualType &Type, ParsedAttr &PAttr) {
+  assert(PAttr.getKind() == ParsedAttr::AT_WebAssemblyFuncref);
----------------
Just to avoid confusing editors that can't tell `Type` is a variable and not `clang::Type` the type.


================
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}}
 
----------------
Should we also test two-level application of the qualifier, as in?
```
using uh_oh = funcref_t __funcref;
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128440/new/

https://reviews.llvm.org/D128440