[PATCH] D43933: [WebAssembly] Pass ownership of body to SyntheticFunction and assert type. NFC

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: wasm/InputChunks.h:156
+  SyntheticFunction(const WasmSignature &S, std::string Body, StringRef Name)
+      : InputFunction(S, nullptr, nullptr), Name(Name), Body(std::move(Body)) {}
 
----------------
Seems reasonable.  I'm not sure if the ownership passing and move semantics stuff is overkill here in lld land though.  Seems like make<> and the arenaAllocator might be preferred/simpler.  I'll defer to @ruiu 


================
Comment at: wasm/Writer.cpp:870
     }
     writeU8(OS, OPCODE_END, "END");
 
----------------
The block scope here was intended to automatically flush and destroy the OS object. 

I suggest you move BodyContent outside this block, then create a second block for OS2 (in which you can actually call it OS, since it will be separate scope. 

The OS2.flush() below is redundant since the scope will trigger that flush when the stream is destroyed.

Seems strange that FunctionBody and BodyContent are not the same type.   Can they be?


================
Comment at: wasm/Writer.cpp:899
+      FunctionSymbol *Sym = File->getFunctionSymbol(F.Symbol);
+      assert(*Sym->getFunctionType() == VoidSignature);
+      InitFunctions.emplace_back(WasmInitEntry{Sym, F.Priority});
----------------
How about just `assert(*Sym->getFunctionType() == WasmSignature{{}, WASM_TYPE_NORESULT});`

Then the release build won't need the extra variable.

This seems like its really a separate change.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D43933