[PATCH] D44206: [WebAssembly] Refactor order of creation for SyntheticFunction

Rui Ueyama via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 7 10:54:39 PST 2018


ruiu added inline comments.


================
Comment at: wasm/InputChunks.h:159-160
   StringRef getComdat() const override { return StringRef(); }
 
+  void setBody(ArrayRef<uint8_t> Body_) { Body = Body_; }
+
----------------
sbc100 wrote:
> ncw wrote:
> > ruiu wrote:
> > > I don't actually know if this is a good thing. Currently a syntehtic function is immutable, but now with this change it is mutable. It is not a good thing in general.
> > There's an upside and a downside - previously we were mutating the DefinedFunction symbol object. So there's no extra mutation, it's just shifted from one object to another. It's "safer" I think to mutate the SyntheticFunction, since nothing looks at its body until the very end (when the CODE section is written out), whereas it's more dangerous to mutate the symbol, since various fields are retrieved from it earlier on.
> I agree with @ncw here.  Previously we were mutating the symbol to point to an entirely new function, not we are only mutating the body of the function and leaving the symbol alone.
> 
> I can see more refinements here down the road but think this is step forward.  
I don't like the fact that this class has two ways to set body. But you can eliminate Body from the constructor , no?


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D44206





More information about the llvm-commits mailing list