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

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi marked 15 inline comments as done.
plotfi 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);
----------------
probinson wrote:
> 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.
I was actually going to rewrite this using std::string since CompressedDebugSection could just own the new string name.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:727-728
 
-static void ExecuteElfObjcopyOnArchive(const CopyConfig &Config, const Archive &Ar) {
+static void ExecuteElfObjcopyOnArchive(const CopyConfig &Config,
+                                       const Archive &Ar) {
   std::vector<NewArchiveMember> NewArchiveMembers;
----------------
jhenderson wrote:
> Do this in a separate NFC commit.
I will do an NFC Commit without starting another review the, FYI. 


Repository:
  rL LLVM

https://reviews.llvm.org/D49678