[PATCH] D43496: [WebAssembly] Split addDefined into two different methods. NFC.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: wasm/SymbolTable.cpp:150
+
+static bool shouldReplace(const Symbol &Existing, const NewSymbol &New) {
   bool Replace = false;
----------------
sbc100 wrote:
> ruiu wrote:
> > We can just pass File, Flags, Chunk and IsFunction to this class, then we don't need NewSymbol class, no?
> (I assume you mean "..to this function).
> 
> Thats how it was before but I found it easier to read this way, since all the properties of the new symbol are under `New` this way, and i makes the call site a little more readable (to me).   I can revert if you prefer.  
> 
> Also, the symbol table change is going to add another field (or anther param) here for input global (that don't use chunks).
It feels a bit odd to me to define a struct to pack four arguments into one argument, so let's avoid doing that. If you prefer, you could name these parameters NewFile, NewFlags, NewChunk and NewIsFunction so that you could use them instead of New.File, New.Flags, New.Chunk and New.IsFunction.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D43496