[PATCH] D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections.

[Paul Robinson via Phabricator via llvm-commits]

probinson added inline comments.


================
Comment at: lib/Object/Compressor.cpp:34
+
+  static std::map<std::string, std::tuple<std::string, std::string>> NameMap;
+  auto I = NameMap.find(LookupName);
----------------
jhenderson wrote:
> jakehehrlich wrote:
> > This is not a bad way to solve the problem of storage. In fact you'll see StringSaver used throughout LLVM in just this way. If this was an expensive computation I'd be all for it too. I'd like to hear other people's thoughts on this just because I'm a bit cautious of it. I've come to find the StringSaver pattern to be an anti-pattern and an indication that StringRef was being over used. Unclear if its the case here or not.
> Personally, I'm against using static local variables for storage, because it means we can't easily unit-test the function (we'd have to provide some way to reset the state, which is non-trivial for such things). I know that we're not currently unit-testing anything in llvm-objcopy, but I still think we should be.
> 
> I don't have a good alternative solution though except to change ownership of section names, which could get expensive if we have a lot of sections.
This code lives in a library, it should not be using static non-const storage.  StringSaver is typically used as a local variable or class member, and needs an allocator.  I don't see it used as a static anywhere.


Repository:
  rL LLVM

https://reviews.llvm.org/D49678