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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

So the main reason I started this is, we have signatures (`WasmSignature` objects), signature indices (`uint32_t`), and other custom types for other sections (`GlobalType` and `EventType`). And all these three are represented by variable names like 'Types' / '***Types`, and I wanted to use different variable and functions names for each of these to be more readable. But as you said, it's subtle because we can't change the name of 'Type Section', so  ¯\_(ツ)_/¯



================
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; }
----------------
sbc100 wrote:
> 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.
We also renamed it as `Signatures` [[ https://github.com/llvm-mirror/llvm/blob/30d4f3445679429ed28ce12116759ba7b28a2a55/lib/MC/WasmObjectWriter.cpp#L235 | here ]], which motivated me to rename these for the sake of consistency. We can change both (and possibly other places) back to `types` though.


================
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; }
----------------
sbc100 wrote:
> Maybe just functionSignatures?
Then we end up calling both `WasmSignature` objects and indices (`uint32_t`) as 'Signatures'. One of the main purpose of this patch is to distinguish them. If you think that makes things more confusing, I think I can drop this patch.


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


Repository:
  rL LLVM

https://reviews.llvm.org/D54689