[PATCH] D43848: [WebAssembly] Simplify initializeSymbols and merge it with ObjFile::parse. NFC.

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

My comments only relate to the removal of the members and other refactoring.  Maybe you want land this without those extra parts and make that a separate change?



================
Comment at: lld/wasm/InputFiles.cpp:46
 void ObjFile::dumpInfo() const {
+  uint32_t NumFuncs = WasmObj->getNumImportedFunctions();
+  uint32_t NumGlobals = WasmObj->getNumImportedGlobals();
----------------
Call this NumFuncs is a little deceptive since its the number of imported functions, which is doesn't include the defined functions.

Perhaps just inline these calls below to avoid the variable?


================
Comment at: lld/wasm/InputFiles.cpp:168
+  uint32_t N = WasmObj->getNumImportedFunctions();
+  assert(N <= Sym.Info.ElementIndex);
+  return Functions[Sym.Info.ElementIndex - N];
----------------
This reads backwards to me.

I'd prefer it to read "Assert that the symbol's element index is less than the number of imported functions"... which is the same thing as saying "Assert that the symbol points to an imported function rather than a defined one".

Why not keep the more meaningful "NumFunctionImports" name?  Or perhaps just call it twice since the first call is inside of an assert (so won't effect release performance)?


https://reviews.llvm.org/D43848