[PATCH] D150305: ObjCopy: support `--dump-section` on COFF

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd marked 3 inline comments as done.
compnerd added inline comments.


================
Comment at: llvm/lib/ObjCopy/COFF/COFFObjcopy.cpp:135
+  for (const coff::Section &Section : O.getSections()) {
+    if (Section.Name != S) continue;
+
----------------
jhenderson wrote:
> This doesn't look to conform to standard LLVM coding style. Please make sure you have run clang-format on your code.
It does - it just is the older style :). Old habits die hard.  I had avoided running it as it results in a lot of noise due to the removal, but I've run it and it does reflow this (and the other file - which is why I had avoided it.


================
Comment at: llvm/lib/ObjCopy/COFF/COFFObjcopy.cpp:152
+  return createStringError(errc::no_such_file_or_directory,
+                           "Section '%s' not found", S.str().c_str());
+}
----------------
jhenderson wrote:
> Please see the coding standards for errors and warning messages (use lower-case "s" for this message): https://llvm.org/docs/CodingStandards.html#error-and-warning-messages
Probably should do the same in the ELF path :)


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/dump-section.test:1
+# RUN: yaml2obj %s -o %t.obj
+# RUN: llvm-objcopy --dump-section .data=%t.dat %t.obj
----------------
jhenderson wrote:
> Please add the following two test cases: 1) the section does not exist; 2) the file cannot be created (e.g. because you're trying to create a file with the same path as an existing directory).
> 
> Check that llvm-objcopy emits an error with the expected message in these cases.
> 
> It may also be interesting to show that the contents of an existing file on disk are overwritten if it is the target of `--dump-section`.
Should probably add those to the ELF tests as well :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150305