[PATCH] D81097: [llvm-objcopy] Reorder --dump-section before --remove-section for ELF.

[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, once my two comments have been addressed. Also, please replace the PR reference with a real bug URL.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/dump-section-before-add-remove.test:9
+# RUN: llvm-objcopy --dump-section .test2=%t1 -R .test2 %t %t2
+# RUN: od -t x1 %t1 | FileCheck %s --ignore-case --match-full-lines
+
----------------
Ktwu wrote:
> alexshap wrote:
> > nit: to me it's somehow easier to read the test when the "check:" lines follow the corresponding invocations:
> > 
> > ```
> >    # RUN: llvm-objcopy ...
> >    # RUN: llvm-objdump %t | FileCheck %s
> > 
> >    # CHECK: Expected line 1
> >    # CHECK: Expected line 2
> > 
> >    !ELF
> >    FileHeader:
> >       Class: ...
> > ```
> > not sure how it compares with what @MaskRay mentioned above, I do not have a strong opinion regarding this.
> > 
> I visually prefer CHECKs after RUNs as well, and it's what folks in lld have been doing, too (and it's also consistent with the NODUMP check below -- the FileCheck happens before the NODUMP).
> 
> </2cents>
+1 - this is also my prefererred style


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/dump-section-before-add-remove.test:6
+
+## Verify that section is dumped before remove.
+# RUN: llvm-objcopy --dump-section .test2=%t1.dump -R .test2 %t %t1
----------------
Nit: "before it is removed".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81097