[PATCH] D41731: [llvm-objcopy] Add --add-gnu-debuglink

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:340-341
+  Name = ".gnu_debuglink";
+  // We need to pick some OriginalOffset and it turns out to be fine to
+  // use a very very large original offset. For sections not found in
+  // segments this data is only used to establish the order that sections
----------------
"turns out to be fine" implies that you don't know the underlying code. I think it might be better to remove this first sentence and simply say "For sections not found in segments, OriginalOffset is only used to establish the order that sections should go in."


================
Comment at: tools/llvm-objcopy/Object.cpp:345
+  // end.
+  OriginalOffset = ~static_cast<uint64_t>(0); // A really big number.
+  JamCRC crc;
----------------
Why not use std::numeric_limits<uint64_t>::max()?


================
Comment at: tools/llvm-objcopy/Object.cpp:365
+  if (!DebugOrErr)
+    error(File + " could not be opened");
+  auto Debug = std::move(*DebugOrErr);
----------------
Could this also supply the appropriate error message, please, e.g. "file not found" or "access denied".


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:121
+static cl::opt<std::string> AddGnuDebugLink("add-gnu-debuglink",
+                                            cl::desc("adds a .gnu_debuglink for <debug-file>"),
+                                            cl::value_desc("debug-file"));
----------------
Has this line been clang-formatted?


Repository:
  rL LLVM

https://reviews.llvm.org/D41731