Page MenuHomePhabricator

[CommandLine] Add inline ArgName printing
ClosedPublic

Authored by dsprenkels on Oct 28 2019, 2:44 AM.

Details

Summary

This patch adds PrintArgInline (after PrintArg) that strips the
leading spaces from an argument before printing them, for usage
inline.

Related bug: PR42943 https://bugs.llvm.org/show_bug.cgi?id=42943

Diff Detail

Event Timeline

dsprenkels created this revision.Oct 28 2019, 2:44 AM
dsprenkels edited the summary of this revision. (Show Details)Oct 28 2019, 2:45 AM

Hi @dsprenkels,

Thanks for the patch. I've just got back from a couple of weeks off, and have a lot to catch up on, but I'll get back to reviewing this in due course (it may be a few days, depending on how things go). In the meantime, you might want to look at who else has recently modified this file and add them as reviewers too.

You also should add one or more tests to show that the bug this issue fixes has been fixed.

Added OptionErrorMessage test case in CommandLineTest for PR42943.

@jhenderson, I added the test.

The only person that has modified llvm/lib/Support/CommandLine.cpp a couple of times in the last year is @hintonda. I added them as a reviewer.

@jhenderson, I added the test.

The only person that has modified llvm/lib/Support/CommandLine.cpp a couple of times in the last year is @hintonda. I added them as a reviewer.

Thanks for the patch.

I originally added this to cleanup printing help, so I appreciate you fixing the error messages too.

llvm/lib/Support/CommandLine.cpp
142

Instead of adding a new class that's only used 3 times, please consider passing an additional prefix padding variable that defaults to the current behavior, 2, and then pass 0 for the three error messages you fixed.

That way, you can forward the padding down to argPrefix() and clean it up while you're at it -- it could use a bit of refactoring too.

Updated the diff, incorporating @hintonda's suggestions.

I think adding an extra constructor argument to PrintArg might be the way forward. There are several places that add additional spaces before the PrintArg call (see e.g. printGenericOptionDiff and printOptionInfo in generic_parser_base), where the spaces could be combined with the two implicitly added currently, I think by specifying, e.g. "4" as the extra argument. The alternative is to go the other way and remove the spaces that PrintArg automatically adds, and then add them in the relevant places explicitly. I'm not sure how clean that would be though.

Not that it matters, but something odd has happened with your context: the CommandLineTest.cpp file is missing all context after your change. I suspect it's the end of the file, but it might mean you've got a mistake in whatever you are using to create the diff.

llvm/lib/Support/CommandLine.cpp
91–94

This should be static.

106

Nit: size_t might be more appropriate, since you're dealing with the size of a data structure here. Probably the same goes for the Pad variables.

1456

You should have a test for this change too, if feasible.

llvm/unittests/Support/CommandLineTest.cpp
1658–1659

I'd delete this reference to the bugzilla. It'll go stale very quickly, especially as we might well move to Github issues very soon, and doesn't really add anything.

Updated the diff incorporating the new comments.

Updated the diff to one generated in the correct way.

@jhenderson, you were right. I forgot how to upload patches correctly, and that is how the context got lost. The new diff (226891) should be exported correctly. Thanks for the heads-up.

jhenderson accepted this revision.Oct 29 2019, 8:04 AM

Aside from one formatting nit, this looks good to me. Please let @hintonda provide any more comments though before this gets committed.

llvm/lib/Support/CommandLine.cpp
1456

As an aside, once you've addressed a review comment, it's helpful if you select the "Done" button for each of those comments. You can do this prior to uploading the diff and they will get submitted when you upload too.

llvm/unittests/Support/CommandLineTest.cpp
1700

Did you remember to run clang-format?

This revision is now accepted and ready to land.Oct 29 2019, 8:04 AM
dsprenkels marked an inline comment as done.

Fixed formatting using clang-format.

As an aside, once you've addressed a review comment, it's helpful if you select the "Done" button for each of those comments. You can do this prior to uploading the diff and they will get submitted when you upload too.

As you might have noticed, this is my first review on Phabricator. Thanks for your patience. :)

LGTM, thanks for the patch!

I see you haven't landed this yet. Do you have commit rights? If not, I'd be happy to land it for you.

dsprenkels marked 6 inline comments as done.Nov 5 2019, 12:11 AM

I don't have commit right. This is one of my first patches. Can you do it for me please?

This revision was automatically updated to reflect the committed changes.