Page MenuHomePhabricator

[yaml2obj] Refactor command line parsing
ClosedPublic

Authored by MaskRay on Feb 4 2020, 10:58 AM.

Details

Summary
  • Hide unrelated options.
  • Add "OVERVIEW: " to yaml2obj -h/--help.
  • Place options under a yaml2obj category.
  • Disallow -docnum. Currently -docnum is the only yaml2obj specific long option that is affected.
  • Specify cl::init("-") and cl::Prefix for OutputFilename. The latter allows -ofile

Diff Detail

Event Timeline

MaskRay created this revision.Feb 4 2020, 10:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2020, 10:58 AM

Unit tests: unknown.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

MaskRay updated this revision to Diff 242381.Feb 4 2020, 11:06 AM
MaskRay edited the summary of this revision. (Show Details)
MaskRay removed a subscriber: merge_guards_bot.

Add cl::Prefix

Unit tests: unknown.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

MaskRay updated this revision to Diff 242385.Feb 4 2020, 11:33 AM

<input> -> <input file>

Unit tests: unknown.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

With this patch applied I observe the different behavior between --help-list and -help-list:

umb@ubuntu:~/LLVM/LLVM/llvm-project/build/bin$ ./yaml2obj -help-list
OVERVIEW: Create an object file from YAML description
USAGE: yaml2obj [options] <input file>

OPTIONS:

Generic Options:

  --help          - Display available options (--help-hidden for more)
  --help-list     - Display list of available options (--help-list-hidden for more)
  --version       - Display the version of this program

yaml2obj Options:

  --docnum=<uint> - Read specified document from input (default = 1)
  -o=<filename>   - Output filename
umb@ubuntu:~/LLVM/LLVM/llvm-project/build/bin$ ./yaml2obj --help-list
OVERVIEW: Create an object file from YAML description
USAGE: yaml2obj [options] <input file>

OPTIONS:
  --docnum=<uint> - Read specified document from input (default = 1)
  --help          - Display available options (--help-hidden for more)
  -o=<filename>   - Output filename
  --version       - Display the version of this program

The original behavior was:

umb@ubuntu:~/LLVM/LLVM/llvm-project/build/bin$ ./yaml2obj --help-list
USAGE: yaml2obj [options] <input>

OPTIONS:
  --color         - Use colors in output (default=autodetect)
  --docnum=<uint> - Read specified document from input (default = 1)
  --help          - Display available options (--help-hidden for more)
  -o=<filename>   - Output filename
  --version       - Display the version of this program
umb@ubuntu:~/LLVM/LLVM/llvm-project/build/bin$ ./yaml2obj -help-list
USAGE: yaml2obj [options] <input>

OPTIONS:
  --color         - Use colors in output (default=autodetect)
  --docnum=<uint> - Read specified document from input (default = 1)
  --help          - Display available options (--help-hidden for more)
  -o=<filename>   - Output filename
  --version       - Display the version of this program
llvm/test/tools/yaml2obj/help.test
4

I'd use "# RUN:" for consistency with our other tests.

14

Would --implicit-check-not="Options:" be a better way to test it?

llvm/test/tools/yaml2obj/output-file.yaml
6

The test name is "invalid output file", but the new test case checks the
valid usage of the "-o" option isn't?

Perhaps you can rename the test file and keep the new test here.
Also it probably deserves a comment about what is tested.

18

Misaligned.

MaskRay updated this revision to Diff 242520.Feb 5 2020, 12:15 AM
MaskRay marked 5 inline comments as done.

Address comments

With this patch applied I observe the different behavior between --help-list and -help-list:

umb@ubuntu:~/LLVM/LLVM/llvm-project/build/bin$ ./yaml2obj -help-list
OVERVIEW: Create an object file from YAML description
USAGE: yaml2obj [options] <input file>

OPTIONS:

Generic Options:

  --help          - Display available options (--help-hidden for more)
  --help-list     - Display list of available options (--help-list-hidden for more)
  --version       - Display the version of this program

yaml2obj Options:

  --docnum=<uint> - Read specified document from input (default = 1)
  -o=<filename>   - Output filename
umb@ubuntu:~/LLVM/LLVM/llvm-project/build/bin$ ./yaml2obj --help-list
OVERVIEW: Create an object file from YAML description
USAGE: yaml2obj [options] <input file>

OPTIONS:
  --docnum=<uint> - Read specified document from input (default = 1)
  --help          - Display available options (--help-hidden for more)
  -o=<filename>   - Output filename
  --version       - Display the version of this program

The original behavior was:

umb@ubuntu:~/LLVM/LLVM/llvm-project/build/bin$ ./yaml2obj --help-list
USAGE: yaml2obj [options] <input>

OPTIONS:
  --color         - Use colors in output (default=autodetect)
  --docnum=<uint> - Read specified document from input (default = 1)
  --help          - Display available options (--help-hidden for more)
  -o=<filename>   - Output filename
  --version       - Display the version of this program
umb@ubuntu:~/LLVM/LLVM/llvm-project/build/bin$ ./yaml2obj -help-list
USAGE: yaml2obj [options] <input>

OPTIONS:
  --color         - Use colors in output (default=autodetect)
  --docnum=<uint> - Read specified document from input (default = 1)
  --help          - Display available options (--help-hidden for more)
  -o=<filename>   - Output filename
  --version       - Display the version of this program

--help-list is -h -e -l -p ... now. ParseCommandLineOptions prints help information after parsing -h and quits, so -e is not rejected.

This change is to make cl::Prefix short options more reliable (cannot conflict with long option names). (We only have one yaml2obj specific long option, so there should not be any problem..)

llvm/test/tools/yaml2obj/output-file.yaml
6

It was already renamed :)

Unit tests: unknown.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

jhenderson added inline comments.Feb 5 2020, 6:32 AM
llvm/test/tools/yaml2obj/help.test
5

Delete --check-prefixes=CHECK,LIST

llvm/test/tools/yaml2obj/output-file.yaml
12

Actually, "No such file or directory" is only OS-dependent via the capitalization of the first letter. We have lots of other tests that check the message, using a regex for that first letter.

llvm/tools/yaml2obj/yaml2obj.cpp
31

I thought using static was more in keeping with LLVM-style?

51

from a YAML

MaskRay updated this revision to Diff 242662.Feb 5 2020, 9:34 AM
MaskRay marked 3 inline comments as done.

Address review comments

llvm/tools/yaml2obj/yaml2obj.cpp
31

LLVM style prefers static.

However, namespaces are used in other places, e.g. llvm-nm. llvm-readobj interestingly uses a named namespace.

static makes the code a bit more verbose but I can change you are strong about this.

Unit tests: fail. 62501 tests passed, 8 failed and 844 were skipped.

failed: LLVM.tools/yaml2obj/help.test
failed: lld.ELF/compressed-input-alignment.test
failed: lld.ELF/invalid/bad-reloc-target.test
failed: lld.ELF/invalid/common-symbol-alignment.test
failed: lld.ELF/invalid/dynamic-section-broken.test
failed: lld.ELF/invalid/symtab-sh-info.s
failed: lld.ELF/mips-elf-flags-err.test
failed: lld.ELF/mips-fp-flags-err.test

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

MaskRay updated this revision to Diff 242688.Feb 5 2020, 10:29 AM
MaskRay added a subscriber: merge_guards_bot.

Rebase to re-trigger @merge_guards_bot

jhenderson accepted this revision.Feb 6 2020, 12:42 AM

LGTM, once @grimar is happy.

llvm/tools/yaml2obj/yaml2obj.cpp
31

Frankly, I prefer namespaces. I don't know why LLVM style is for static.

This revision is now accepted and ready to land.Feb 6 2020, 12:42 AM
grimar accepted this revision.Feb 6 2020, 1:03 AM

LGTM

MaskRay updated this revision to Diff 242826.Feb 6 2020, 1:08 AM

Fix a test

This revision was automatically updated to reflect the committed changes.