[PATCH] D54689: [WebAssembly] Rename function types to signatures

[Sam Clegg via Phabricator via llvm-commits]

sbc100 accepted this revision.
sbc100 added a comment.
This revision is now accepted and ready to land.

lgtm %nits



================
Comment at: include/llvm/Object/Wasm.h:133
   const wasm::WasmDylinkInfo &dylinkInfo() const { return DylinkInfo; }
-  ArrayRef<wasm::WasmSignature> types() const { return Signatures; }
-  ArrayRef<uint32_t> functionTypes() const { return FunctionTypes; }
+  ArrayRef<wasm::WasmSignature> signatures() const { return Signatures; }
+  ArrayRef<uint32_t> functionSigIndices() const { return FunctionSigIndices; }
----------------
Its a little tricky because these corresponds exactly to the wasm "Types" section, which is how this terminology crept in.   I suppose its ok for llvm to deviate from the wasm spec here.


================
Comment at: include/llvm/Object/Wasm.h:134
+  ArrayRef<wasm::WasmSignature> signatures() const { return Signatures; }
+  ArrayRef<uint32_t> functionSigIndices() const { return FunctionSigIndices; }
   ArrayRef<wasm::WasmImport> imports() const { return Imports; }
----------------
Maybe just functionSignatures?


================
Comment at: lib/MC/WasmObjectWriter.cpp:329
+  uint32_t registerFunctionSignature(const MCSymbolWasm &Symbol);
+  uint32_t registerEventSignature(const MCSymbolWasm &Symbol);
 };
----------------
Looks like the return of these two is never uses, perhaps they should just return void.. not part of this CL though.


================
Comment at: test/ObjectYAML/wasm/event_section.yaml:18
   - Type:            FUNCTION
-    FunctionTypes:   [ 0 ]
+    FunctionSigIndices:   [ 0 ]
   - Type:            EVENT
----------------
I think I prefer just "Signatures" or "FunctionSignatures"


================
Comment at: tools/obj2yaml/wasm2yaml.cpp:165
       uint32_t Index = 0;
-      for (const auto &FunctionSig : Obj.types()) {
+      for (const auto &FunctionSig : Obj.signatures()) {
         WasmYAML::Signature Sig;
----------------
Just "Signature" here since this could be an exception sig now right?


================
Comment at: tools/yaml2obj/yaml2wasm.cpp:277
     if (Sig.Index != ExpectedIndex) {
-      errs() << "Unexpected type index: " << Sig.Index << "\n";
+      errs() << "Unexpected signature index: " << Sig.Index << "\n";
       return 1;
----------------
Hmm.. another place where diverging from the spec makes this is little less readable I think.


Repository:
  rL LLVM

https://reviews.llvm.org/D54689