[PATCH] D62072: Worked on bug 40859 to be more consistent with error messages in llvm-objcopy

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, aside from the inline comments. I'd like @jakehehrlich to take a look, given he's the code owner and has final say on these things, but if he doesn't get around to it, I'm happy to commit it tomorrow.

With regards to clang-formatting, make sure to keep the changes to only areas you've touched, as this makes git blame etc harder otherwise (see also the Developer Policy).



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/strip-group-symbol.test:31
 
-#CHECK: Symbol foo cannot be removed because it is referenced by the section .group[1].
+#CHECK: symbol 'foo' cannot be removed because it is referenced by the section '.group[1]'
----------------
Not related to your change, but you can tidy it up whilst you're here. I think most people find it easier to read the lines with a space between '#' and 'CHECK', so please add it. Same applies to strip-reloc-symbol.test.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:189-190
 template <class... Ts>
-static Error makeStringError(std::error_code EC, const Twine &Msg, Ts&&... Args) {
+static Error makeStringError(std::error_code EC, const Twine &Msg,
+                             Ts &&... Args) {
   std::string FullMsg = (EC.message() + ": " + Msg).str();
----------------
If this isn't something you've touched directly, don't change the formatting, even if it's wrong (small changes like this are okay in separate commits though if you're working in the general area).


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:254
   };
-  if (Error E = DWOFile->removeSections(Config.AllowBrokenLinks,
-                                        OnlyKeepDWOPred))
+  if (Error E =
+          DWOFile->removeSections(Config.AllowBrokenLinks, OnlyKeepDWOPred))
----------------
abrachet wrote:
> clang-format did this but I think the previous one looks better
I'd revert it, since this isn't related to your change (see above comment).


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:538-540
+    replaceDebugSections(
+        Obj, RemovePred, isCompressable, [&Config, &Obj](const SectionBase *S) {
+          return &Obj.addSection<CompressedSection>(*S, Config.CompressionType);
----------------
See above, revert this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62072