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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/wasm/InputFiles.cpp:168
+  uint32_t N = WasmObj->getNumImportedFunctions();
+  assert(N <= Sym.Info.ElementIndex);
+  return Functions[Sym.Info.ElementIndex - N];
----------------
sbc100 wrote:
> 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)?
The reason why I removed these member variables is because they don't do anything expensive and actually as cheap as member variables. They are defined as shown below.

  uint32_t getNumImportedGlobals() const { return NumImportedGlobals; }
  uint32_t getNumImportedFunctions() const { return NumImportedFunctions; }

So, what "caching" results of these functions does is to make extra copies of these member variables in our instances, which doesn't make much sense to me. I'm not worried about the cost of having extra copies, but if we have two copies of the same data, that is error prone.


https://reviews.llvm.org/D43848