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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Some more comments, but there are unaddressed comments from earlier reviews as well.



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


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11804
+def err_wasm_funcref_not_wasm : Error<
+  "invalid use of __funcref keyword outside the WebAssembly triple">;
 } // end of sema component.
----------------



================
Comment at: clang/lib/AST/DeclBase.cpp:1060-1063
+  if (Ty->isFunctionPointerType())
+    return true;
+
+  return false;
----------------



================
Comment at: clang/lib/Parse/ParseDecl.cpp:854-855
+  SourceLocation AttrNameLoc = ConsumeToken();
+  attrs.addNew(AttrName, AttrNameLoc, /*ScopeName=*/nullptr, /*ScopeLoc=*/SourceLocation{}, /*Args=*/nullptr, /*numArgs=*/0,
+               ParsedAttr::AS_Keyword);
+}
----------------
Be sure to run clang-format over the patch to fix this sort of thing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128440