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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
