[PATCH] D61993: [llvm-objcopy] Add file names to error messages

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Another thing is that you should make sure that all the code paths you modified are tested where possible. The tests probably already exist, and just need updating to check that the filename appears.



================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:688
     if (Error E = dumpSectionToFile(SecName, File, Obj))
-      return createFileError(Config.InputFilename, std::move(E));
+      return createFileError(File, std::move(E));
   }
----------------
I don't have a problem with this change, but why did you make it? Also, it should probably be a separate patch, since it's not related to the bug you are trying to fix.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:737
   if (!Config.BuildIdLinkDir.empty()) {
-    BuildIdBytes = unwrapOrError(findBuildID(In));
+    auto BuildIdBytesOrErr = findBuildID(In);
+    if (auto E = BuildIdBytesOrErr.takeError())
----------------
Rather than create the file error here, it might make more sense to call createFileError in findBuildId.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:751
     if (Error E =
             linkToBuildIdDir(Config, Config.InputFilename,
                              Config.BuildIdLinkInput.getValue(), BuildIdBytes))
----------------
linkToBuildIdDir already calls createFileError in some situations. Rather than repeat it here, it would be better to change any necessary return sites in that function.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61993