[PATCH] D71269: [llvm-readobj][test] - Cleanup and split tests in tools/llvm-readobj folder.

George Rimar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Dec 11 05:31:56 PST 2019


grimar added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/COFF/hex-dump.test:2
+## Test that -x can be used to display the contents
+## of the indicated section as a hexadecimal bytes.
+
----------------
jhenderson wrote:
> Delete "a".
> 
> The comment says it's testing that the alias can be used, but what about the normal switch?
I've added a single line check for a normal switch.

BTW, I think almost every test that was splitted can/should be improved.
The main intention of this patch is just to split the tests here and move them to
corresponding folders to open the road for further changes though.


================
Comment at: llvm/test/tools/llvm-readobj/COFF/hex-dump.test:4
+
+## Below we test -x can be used for other binary formats.
+
----------------
jhenderson wrote:
> This comment doesn't make sense any more.
Removed.


================
Comment at: llvm/test/tools/llvm-readobj/COFF/sections-ext.test:1-2
+## This is a test case for --section-symbols, --section-relocations
+## and --section-data command line options.
+
----------------
jhenderson wrote:
> The comment uses the long-form, but the test only uses the short-form. We should test both probably.
Done.


================
Comment at: llvm/test/tools/llvm-readobj/MachO/hex-dump.test:2
+## Test that -x can be used to display the contents
+## of the indicated section as a hexadecimal bytes.
+
----------------
jhenderson wrote:
> Delete "a".
> 
> The comment says it's testing that the alias can be used, but what about the normal switch?
I've added a test.


================
Comment at: llvm/test/tools/llvm-readobj/MachO/sections-ext.test:1-2
+## This is a test case for --section-symbols, --section-relocations and
+## --section-data command line options.
+
----------------
jhenderson wrote:
> The comment uses the long-form, but the test only uses the short-form. We should test both probably.
I've added tests for long-form.


================
Comment at: llvm/test/tools/llvm-readobj/wasm/hex-dump.test:2
+## Test that the -x alias of --hex-dump can be used to display the contents
+## of the indicated section as a hexadecimal bytes.
+
----------------
jhenderson wrote:
> Delete "a".
> 
> The comment says it's testing that the alias can be used, but what about the normal switch?
I've aaded a switch and updated the comment.


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

https://reviews.llvm.org/D71269





More information about the llvm-commits mailing list