aaron.ballman requested changes to this revision. aaron.ballman added a comment. This revision now requires changes to proceed.
Same question here about an RFC for adding this new type to the type system and a design document for how the type behaves as in https://reviews.llvm.org/D122215. ================ Comment at: clang/include/clang/Basic/Attr.td:4053 + let Spellings = [Keyword<"__funcref">]; + let Documentation = [Undocumented]; +} ---------------- tlively wrote: > It would be good to document this! It's a requirement that this be documented. Also, this should probably have a `Subjects` line that expects a function of some sort? (It doesn't impact the semantics of anything, but it self-documents the expectations for people reading Attr.td.) ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7332 +def err_attribute_webassembly_funcref : Error< + "'__clang_webassembly_funcref' attribute can only be applied to function pointer types">; ---------------- 1) What is `__clang_webassembly_funcref`? Did you mean `__funcref`? 2) There's no test coverage for this diagnostic. ================ Comment at: clang/include/clang/Basic/TokenKinds.def:666 +// WebAssembly Type Extension +KEYWORD(__funcref , KEYALL) + ---------------- Why is this a keyword in all language modes? ================ Comment at: clang/include/clang/Parse/Parser.h:2843 void ParseMicrosoftTypeAttributes(ParsedAttributes &attrs); + void ParseWebAssemblyFuncrefTypeAttribute(ParsedAttributes &attrs); void DiagnoseAndSkipExtendedMicrosoftTypeAttributes(); ---------------- ================ Comment at: clang/include/clang/Sema/Sema.h:13103-13104 CallExpr *TheCall); + bool CheckWebAssemblyBuiltinFunctionCall(const TargetInfo &TI, unsigned BuiltinID, + CallExpr *TheCall); ---------------- Formatting is off here. ================ Comment at: clang/include/clang/Sema/Sema.h:13169 + // WebAssembly builtin handling + bool SemaBuiltinWasmRefNullFunc(CallExpr *TheCall); + ---------------- I think there's a verb missing from this function identifier -- also, I don't think we need to continue the terrible trend of prepending `Sema` onto the function name which lives in the `Sema` object. ================ Comment at: clang/lib/AST/ASTContext.cpp:954 10, // wasm_externref, + 20, // wasm_funcref }; ---------------- Where did this value come from? ================ Comment at: clang/lib/Parse/ParseDecl.cpp:845-846 +void Parser::ParseWebAssemblyFuncrefTypeAttribute(ParsedAttributes &attrs) { + if (Tok.getKind() == tok::kw___funcref) { + IdentifierInfo *AttrName = Tok.getIdentifierInfo(); ---------------- ================ Comment at: clang/lib/Parse/ParseDecl.cpp:851-854 + } else { + return; + } +} ---------------- ================ Comment at: clang/lib/Parse/ParseDecl.cpp:5780 + case tok::kw___funcref: + if (AttrReqs & AR_DeclspecAttributesParsed) { + ParseWebAssemblyFuncrefTypeAttribute(DS.getAttributes()); ---------------- Why that specific ordering as opposed to `AR_VendorAttributesParsed`? ================ Comment at: clang/lib/Sema/SemaChecking.cpp:6557-6561 + // Therefore we need to change the types of the DeclRefExpr (stored in FDecl) + // and regenerate a straight up CallExpr on the modified FDecl. + // returning + // CallExpr + // `- FunctionDecl ---------------- Why would we need to do this?? And what do we do when there is no `FunctionDecl` to get back to (because it's a call expression through a function pointer, for example)? ================ Comment at: clang/lib/Sema/SemaChecking.cpp:6564 + // Prepare FDecl type + QualType Pointee = Context.getFunctionType(Context.VoidTy, {}, {}); + QualType Type = Context.getPointerType(Pointee); ---------------- Why are you guaranteed this function returns void? ================ Comment at: clang/lib/Sema/SemaType.cpp:7256 + QualType Desugared = Type; + const AttributedType *AT = dyn_cast<AttributedType>(Type); + while (AT) { ---------------- ================ Comment at: clang/lib/Sema/SemaType.cpp:7269 + } + Attrs[NewAttrKind] = true; + ---------------- This seems like it's not used. ================ Comment at: clang/lib/Sema/SemaType.cpp:7289-7290 + QualType Pointee = Type->getPointeeType(); + Pointee = S.Context.getAddrSpaceQualType( + S.Context.removeAddrSpaceQualType(Pointee), ASIdx); + Type = State.getAttributedType(A, Type, S.Context.getPointerType(Pointee)); ---------------- What happens when the user's function already has an explicitly specified address space? 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
