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

[Nicholas Wilson via Phabricator via llvm-commits]

ncw added inline comments.


================
Comment at: wasm/Writer.cpp:602
+  bool ExportHidden = Config->EmitRelocs;
+  std::set<std::string> UsedNames;
+  auto BudgeName = [&](const Symbol *Sym) {
----------------
sbc100 wrote:
> ncw wrote:
> > sbc100 wrote:
> > > 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.
> > `DenseSet<StringRef>` and `StringSet` both assume that the string itself is owned externally. But the "budged" names can't be stored just as a StringRef, something needs to actually own the buffer to the string itself, hence using `std::string`. And you can't use a StringRef to refer to things in a `vector<std::string>` either (can you?) because you have to careful mutating the vector not to reallocate the strings and invalidate any StringRefs (I suppose we can rely on move ctors these days).
> > 
> > Nuisance. I don't suppose it's good style to use `make<std::string>` or similar magic to allocate strings?
> > 
> > I could maybe see about doing some more explicit buffer management to allow use of an `llvm::StringSet`.
> In that case std::set seems reasonable to me.
Aha! With some more searching, I've found the idiomatic LLVM solution.

It's called `llvm::Saver`, a global instance of `llvm::StringSaver` that lets you get a safe/stable StringRef for a fresh string.

Updated now to use that.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D42105