[PATCH] D42105: [WebAssembly] Symbol changes #4: export relocatable, LLD only

[Sam Clegg via Phabricator via llvm-commits]

sbc100 accepted this revision.
sbc100 added a comment.
This revision is now accepted and ready to land.

Just a couple of minor nits



================
Comment at: test/wasm/Inputs/locals-duplicate1.ll:1
+; Will collide: local with public
+ at colliding_global1 = internal default global i32 0, align 4
----------------
Are there llvm terms you can use here rather than `local` and `public`?  I think llvm this `internal` and `external` linkage, right?


================
Comment at: test/wasm/locals-duplicate.test:1
+; RUN: llc -filetype=obj -mtriple=wasm32-unknown-unknown-wasm %p/Inputs/locals-duplicate1.ll -o %t1.o
+; RUN: llc -filetype=obj -mtriple=wasm32-unknown-unknown-wasm %p/Inputs/locals-duplicate2.ll -o %t2.o
----------------
Can we give these new files better names?   Perhaps `duplicate-symbols-internal.test`.  Locals makes me think of wasm locals, and llvm doesn't use the term local for these symbols either (I think?).


================
Comment at: wasm/Writer.cpp:602
+  bool ExportHidden = Config->EmitRelocs;
+  std::set<std::string> UsedNames;
+  auto BudgeName = [&](const Symbol *Sym) {
----------------
ncw wrote:
> I realise that `std::set<std::string>` isn't very idiomatic in LLVM, but you can why I chose it. I couldn't think of another way of doing it though that didn't require more code doing explicit buffer management.
> 
> Suggestions welcome - otherwise it's hopefully "good enough" for now, especially as it's short-lived and only used for exported functions (which I'd expect to be a small number compared to the overall output size).
I think `llvm::DenseSet<StringRef>` is used elsewhere in lld and probably makes sense here.  Or llvm::StringSet?  I'm not sure what the tradeoffs are between those two.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D42105