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

[Derek Schuff via Phabricator via llvm-commits]

dschuff accepted this revision.
dschuff added a comment.

At a high level (i.e. across all the LLVM targets) the intention is that binary and asm streamers have the same interface. That's sort of enforced by the fact that they both have the same base class, but I guess in actuality the "interface" also includes the data on the symbol object. I think I agree that it makes more sense to think of the symbols as owned outside the streamer and have them be a way to pass info into the streamer  (the current code is more like having them be state that is only needed by the binary streamer but which lives outside the streamer). I guess that's also another way of saying that symbol type and module name are properties of a symbol even if those properties are only needed by one of the streamers. I guess it might also be useful for debugging or future modifications to have the contents of the symbols be the same regardless of which output type is in use.

So... I guess that's a long-winded way of saying LGTM?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55347