[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];
----------------
ruiu wrote:
> ruiu wrote:
> > 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.
> Oh sorry I misread your comment. To me
> 
>   uint32_t N = WasmObj->getNumImportedFunctions();
>   assert(Sym.Info.ElementIndex >= N);
>   return Functions[Sym.Info.ElementIndex - N];
> 
> is easier to read than
> 
>   assert(Sym.Info.ElementIndex >= WasmObj->getNumImportedFunctions());
>   return Functions[Sym.Info.ElementIndex - WasmObj->getNumImportedFunctions()];
>  
> because the latter is longer, and it isn't obvious to me that the two function calls are actually the same.
> 
> The reason why I wrote N <= something instead of something >= N is because I'm always thinking on the number line. N is left if it is smaller than the other.
> 
> But that's just my preference. If you feel strongly about it I can revert it.
On second thought, I think we don't really need this assert. If assertion is enabled, out of bound should be caught by the container, and no explicit bound checking isn't needed.


https://reviews.llvm.org/D43848