Page MenuHomePhabricator

[Bash-autocompletion] Show HelpText with possible flags
ClosedPublic

Authored by yamaguchi on Jul 21 2017, 8:21 PM.

Details

Summary

clang --autocomplete=-std will show

-std:   Language standard to compile for
-std=   Language standard to compile for
-stdlib=        C++ standard library to use

by this change.

However, showing HelpText with completion in bash seems super tricky, so
this feature will be used in other shells(fish, zsh...).

Diff Detail

Repository
rL LLVM

Event Timeline

yamaguchi created this revision.Jul 21 2017, 8:21 PM
ruiu added inline comments.Jul 21 2017, 8:51 PM
clang/lib/Driver/Driver.cpp
1302 ↗(On Diff #107773)

Now that the separator and the terminator are the same, there's no reason to use llvm::join. Since llvm::join creates a large temporary string, you want to avoid that if it is easily avoidable.

for (StringRef S : SuggestedCompletions)
  llvm::outs << S << "\n";

If you do this, you might be able to remove StringExtras.h from this file?

llvm/lib/Option/OptTable.cpp
240 ↗(On Diff #107773)

I believe

Ret.push_back(S + "\t" + In.HelpText);

should just work.

yamaguchi updated this revision to Diff 107778.Jul 22 2017, 2:16 AM

Fixed test and update diff according to Rui's comment.

yamaguchi added inline comments.Jul 22 2017, 2:18 AM
llvm/lib/Option/OptTable.cpp
240 ↗(On Diff #107773)

Thanks for pointing out, but I think it segfaults because In.HelpText might be a nullptr.
So I update diff to check if it's nullptr or not, which seems more obvious.

yamaguchi updated this revision to Diff 107779.Jul 22 2017, 2:37 AM

Add newline after the end of the line.

yamaguchi updated this revision to Diff 107781.Jul 22 2017, 5:17 AM

Update diff. Delete trailing whitespace so that -stdlib= autocompletion works correctly.

ruiu added inline comments.Jul 22 2017, 7:30 AM
clang/lib/Driver/Driver.cpp
1303 ↗(On Diff #107781)

You want to print out just one '\n' at end instead of two, no?

yamaguchi updated this revision to Diff 107790.Jul 22 2017, 7:41 AM

Update diff.
Use llvm::join because we don't want to print two '\n's.

ruiu added inline comments.Jul 22 2017, 7:47 AM
clang/utils/bash-autocomplete.sh
60 ↗(On Diff #107790)

\t.*.\s* doesn't seem to make much sense to me. Isn't \t.* enough?

yamaguchi updated this revision to Diff 107791.Jul 22 2017, 8:03 AM

Update diff according to Rui's comment.

ruiu accepted this revision.Jul 22 2017, 8:07 AM

LGTM

So, this patch changes the format of the --autocomplete option in an incompatible way. The bash completion script that will be shipped with LLVM 5.0 will not be able to read the output of --autocomplete after you make this change. Do you want to merge this into 5.0?

This revision is now accepted and ready to land.Jul 22 2017, 8:07 AM

@ruiu
Yeah, we are planning to merge this to 5.0, so that clang Driver interface will be compatible among further versions.

teemperor accepted this revision.Jul 22 2017, 11:50 AM

LGTM.

clang/utils/bash-autocomplete.sh
60 ↗(On Diff #107790)

Can you add some comment why we need the sed? Something like # Filtering out the help texts of the flags as we can't display them in bash easily.

This revision was automatically updated to reflect the committed changes.