Page MenuHomePhabricator

[llvm-dwarfdump] Remove unnecessary explicit -h behaviour
ClosedPublic

Authored by jhenderson on Jun 19 2019, 9:59 AM.

Details

Summary

--help and -h are automatically supported by the command-line parser, unless overridden by the tool. The behaviour of the PrintHelpMessage being used for -h prior to this patch is subtly different to that provided by --help automatically (it omits certain elements of help text and options, such as --help-list), so overriding the default is not desirable, without good reason. This patch removes the explicit specification of -h and its behaviour, so that the default behaviour is used.

Diff Detail

Repository
rL LLVM

Event Timeline

jhenderson created this revision.Jun 19 2019, 9:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2019, 9:59 AM
hintonda accepted this revision.Thu, Jun 20, 7:29 AM

LGTM, but you'll also want to fix llvm/test/Support/check-default-options.txt. It checks your special handling to make sure the default -h support didn't break it.

This revision is now accepted and ready to land.Thu, Jun 20, 7:29 AM

LGTM, but you'll also want to fix llvm/test/Support/check-default-options.txt. It checks your special handling to make sure the default -h support didn't break it.

Thanks, I wasn't aware of that one (and hadn't yet bothered running every lit test for this). Just to be clear, are you recommending that I just delete the bits of that test relating to llvm-dwarfdump, since it will now follow the default process, or do you think there should be something else to replace it? I can replace it with llvm-opt-report, which seems to do something similar to llvm-dwarfdump.

LGTM, but you'll also want to fix llvm/test/Support/check-default-options.txt. It checks your special handling to make sure the default -h support didn't break it.

Thanks, I wasn't aware of that one (and hadn't yet bothered running every lit test for this). Just to be clear, are you recommending that I just delete the bits of that test relating to llvm-dwarfdump, since it will now follow the default process, or do you think there should be something else to replace it? I can replace it with llvm-opt-report, which seems to do something similar to llvm-dwarfdump.

Yes, it can be removed. If you want, you can add it to the ones above that do use the new default -h, but I don't think that's necessary. And yes, if there's another one that does something special, adding it, i.e., s/llvm-dwarfdump/llvm-opt-report/ makes sense.

LGTM, but you'll also want to fix llvm/test/Support/check-default-options.txt. It checks your special handling to make sure the default -h support didn't break it.

Thanks, I wasn't aware of that one (and hadn't yet bothered running every lit test for this). Just to be clear, are you recommending that I just delete the bits of that test relating to llvm-dwarfdump, since it will now follow the default process, or do you think there should be something else to replace it? I can replace it with llvm-opt-report, which seems to do something similar to llvm-dwarfdump.

Yes, it can be removed. If you want, you can add it to the ones above that do use the new default -h, but I don't think that's necessary. And yes, if there's another one that does something special, adding it, i.e., s/llvm-dwarfdump/llvm-opt-report/ makes sense.

Okay, I'm going to remove it and then commit, without replacing the removed test part. My initial evaluation was using an out-of-date llvm-opt-report, and I was unable to find any other suitable candidates. I reviewed the other tools, and the only tools I could find with explicit -h help handling are dsymutil and clang-offload-bundler. In both cases, I think the existing handling is a bug (in dsymutil at least, you actually have to provide a positional argument before it will process '-h'!).

Removed references to llvm-dwarfdump from test/Support/check-default-options.txt.

This revision was automatically updated to reflect the committed changes.

LGTM, but you'll also want to fix llvm/test/Support/check-default-options.txt. It checks your special handling to make sure the default -h support didn't break it.

Thanks, I wasn't aware of that one (and hadn't yet bothered running every lit test for this). Just to be clear, are you recommending that I just delete the bits of that test relating to llvm-dwarfdump, since it will now follow the default process, or do you think there should be something else to replace it? I can replace it with llvm-opt-report, which seems to do something similar to llvm-dwarfdump.

Yes, it can be removed. If you want, you can add it to the ones above that do use the new default -h, but I don't think that's necessary. And yes, if there's another one that does something special, adding it, i.e., s/llvm-dwarfdump/llvm-opt-report/ makes sense.

Okay, I'm going to remove it and then commit, without replacing the removed test part. My initial evaluation was using an out-of-date llvm-opt-report, and I was unable to find any other suitable candidates. I reviewed the other tools, and the only tools I could find with explicit -h help handling are dsymutil and clang-offload-bundler. In both cases, I think the existing handling is a bug (in dsymutil at least, you actually have to provide a positional argument before it will process '-h'!).

That's because they're processing -h after cl::ParseCommandLineOptions returns. But as you say, InputFiles requires at least one argument, so cl::ParseCommandLineOptions never does return if this condition isn't met. The only way to fix that is to remove the special -h processing.

static list<std::string> InputFiles(Positional, OneOrMore,
                                    desc("<input files>"), cat(DsymCategory));