[PATCH] D57007: [llvm-objcopy] [COFF] Implement --add-gnu-debuglink

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/COFF/COFFObjcopy.cpp:54
+                                              StringRef File) {
+  auto LinkTargetOrErr = MemoryBuffer::getFile(File);
+  if (!LinkTargetOrErr)
----------------
mstorsjo wrote:
> jhenderson wrote:
> > This is probably an instance of too much `auto`.
> Ok, will write out the types. (The ELF dir uses `auto` similarly though.)
I know, and I've disagreed with it there in some places too!


================
Comment at: tools/llvm-objcopy/COFF/COFFObjcopy.cpp:57
+    error("'" + File + "': " + LinkTargetOrErr.getError().message());
+  std::unique_ptr<MemoryBuffer> LinkTarget = std::move(*LinkTargetOrErr);
+  uint32_t CRC32 = getCRC32(LinkTarget->getBuffer());
----------------
This one CAN be auto, because its type is clear from the type of LinkTargetOrErr.


================
Comment at: tools/llvm-objcopy/COFF/Object.h:46
+  // instead of OrigContents above.
+  std::vector<uint8_t> Contents;
 };
----------------
Thinking about this a bit more, I wonder if it would be a good idea for there to be a `getContents` function that decides which to fetch. This feels like a potential risk point for bugs, if people try to get `Contents` when it is owned by `OrigContents`. You'd want to make them both private as a result, and therefore add some setters too.


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

https://reviews.llvm.org/D57007