[PATCH] D55347: [WebAssembly] TargetStreamer cleanup (NFC)

[Heejin Ahn via Phabricator via llvm-commits]

aheejin marked 3 inline comments as done.
aheejin added a comment.

In D55347#1321482 <https://reviews.llvm.org/D55347#1321482>, @sbc100 wrote:

> There is a presensent for the "emit" functions in the streamer modifying symbols. For example:
>
>   void MCELFStreamer::emitELFSize(MCSymbol *Symbol, const MCExpr *Value) {         
>     cast<MCSymbolELF>(Symbol)->setSize(Value);                                     
>   }   
>
>
> I'm not saying this is the best, but it seems to have be designed or used this way in other targets, so I'd be tempted to use caution when refactoring.   I myself don't have enough to background to know if this is a direction we want to move in.  @sunfish  and @aardappel  might have a better idea.


I checked streamers of other targets and it was kind of 50/50 (const / non-const). So I don't feel very strongly about putting `const` in there. And I thought `MCSymbolWasm`'s type is a symbol's inherent property, so it is not necessarily be only assocaited with object files. `WebAssemblyMCInstLower` sets types to symbols even before does not know whether it will emit .s or .o file.

But anyway I don't feel strongly about this, so I can revert `setType` and `setModuleName` related part if you suggest so.



================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp:137
-  // Symbol already has its arguments and result set.
-  Symbol->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
-}
----------------
sbc100 wrote:
> We have two different streamers `WebAssemblyTargetWasmStreamer` and `WebAssemblyTargetAsmStreamer`.  I seems that this code is only needed for the object file streamer so moving it out into common code is actually a semantic change.
> 
> Can you revert this part or at least put it in separate change so that this one can be a simple NFC?
If AsmStreamer does not use this property, isn't it NFC anyway? No tests can be affected.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55347/new/

https://reviews.llvm.org/D55347