[GSoC] Shell autocompletion for clang
ClosedPublic

Authored by yamaguchi on May 16 2017, 7:10 AM.

Details

Summary

This is a first patch for GSoC project, bash-completion for clang.
To use this on bash, please run source clang/utils/bash-autocomplete.sh.
bash-autocomplete.sh is code for bash-completion.

Simple flag completion and path completion is available in this patch.

Diff Detail

Repository
rL LLVM
yamaguchi created this revision.May 16 2017, 7:10 AM
yamaguchi updated this revision to Diff 99142.May 16 2017, 7:19 AM

Add reviewers.

yamaguchi edited the summary of this revision. (Show Details)May 16 2017, 7:21 AM
yamaguchi added reviewers: Bigcheese, efriedma.
jroelofs added inline comments.
clang/include/clang/Driver/Options.td
472 ↗(On Diff #99142)

Please add a HelpText so it shows up in the docs.

clang/test/Driver/autocomplete.c
1 ↗(On Diff #99142)

count and FileCheck are preferred over adding new uses of grep.

clang/utils/bash-autocomplete.sh
1 ↗(On Diff #99142)

Please add a commment explaining that this file should be sourced in the user's .bashrc. You also need cmake goop to install this under share/clang.

yamaguchi added inline comments.May 16 2017, 7:52 AM
clang/include/clang/Driver/Options.td
472 ↗(On Diff #99142)

I think this flag should not be show up with --help, because this --autocomplete flag is used only for clang internal.

ruiu added inline comments.May 16 2017, 9:54 AM
clang/utils/bash-autocomplete.sh
6–10 ↗(On Diff #99142)

s/cur"/cur" / for consistency. Or s/$( /$(/ if you prefer, but $( foo) looks odd.

12 ↗(On Diff #99142)

Remove blank line.

13–14 ↗(On Diff #99142)

What is the meaning of && here? It seems you can remove it.

llvm/lib/Option/OptTable.cpp
195 ↗(On Diff #99142)

Remove unnecessary ().

efriedma resigned from this revision.May 16 2017, 12:09 PM
yamaguchi added inline comments.May 16 2017, 10:47 PM
clang/utils/bash-autocomplete.sh
13–14 ↗(On Diff #99142)

syntax error will occur without &&, because for example,
This is correct:
_fun() { echo "hoge"; } && echo "huga"
But this is wrong:
_fun() { echo "hoge"; } echo "huga"

yamaguchi updated this revision to Diff 99248.May 16 2017, 10:50 PM

Update diff.

yamaguchi marked 4 inline comments as done.May 16 2017, 10:50 PM
yamaguchi updated this revision to Diff 99249.May 16 2017, 11:15 PM

Update diff.
Remove &&, because breaking after shell function was enough.

yamaguchi marked 2 inline comments as done.May 16 2017, 11:16 PM
yamaguchi updated this revision to Diff 99250.May 16 2017, 11:23 PM

Update diff.
Add comment in bash-autocomplete.sh.

yamaguchi marked an inline comment as done.May 16 2017, 11:26 PM
jroelofs added inline comments.May 17 2017, 8:06 AM
clang/include/clang/Driver/Options.td
472 ↗(On Diff #99142)

ok, good point.

ruiu added a comment.May 17 2017, 10:37 AM

I'm fine with these nits. (Am I supposed to sign off?)

clang/test/Driver/autocomplete.c
4 ↗(On Diff #99250)

You want to change this to -std={{.*}}-stdlib= to make robust for changes that add new options which are between the top in the asciibetical order.

6 ↗(On Diff #99250)

Maybe just foo suffices. % doesn't seem make sense to me.

clang/utils/bash-autocomplete.sh
14 ↗(On Diff #99250)

Fix indentation.

@ruiu We wanted to push this patch with the follow-up one about the argument completion, so no need to do this now :).

ruiu added a comment.May 17 2017, 10:57 AM

Why do you want to not submit this alone but wait for another patch? In-tree incremental development is a normal thing, and the feature this patch provides is already useful without any further changes, so I don't see a reason to wait.

Because the small follow up patch is relevant for the review of this one. If people disagree with the plan there to extend Options.td or OptTable for this feature (and due to the limitations of Options.td in regards to dynamic arguments, this is already controversial), then we have to completely revisit this code which assumes we can do everything in OptTable. I fully agree with incremental development in tree, but for those first two patches I don't see "consensus on the end goal to define the first increment" yet (to quote the LLVM dev policy on this). Sorry, I should have mentioned this fact in the previous mail, but typing on the smartphone is cumbersome.

ruiu added a comment.May 17 2017, 12:43 PM

If you are adding code to handle dynamic arguments, then yes, we probably need to define what is the first incremental step towards the goal after discussing how to handle dynamic arguments. But this patch seems somewhat orthogonal to that, no? This doesn't do anything about dynamic arguments and makes sense without a follow-up patch. It adds only 12 lines of code to OptTable.cpp, so "completely revising" it shouldn't be a big deal at all. So I believe this is something we usually submit incrementally.

@teemperor, @ruiu, thanks for the quick review and comments! I second the incremental development argument. IMHO several dependent phabricator patches can be considered incremental development.

@ruiu, please feel free give green light if this patch looks good to you.

I'd feel much more comfortable if this patch lands closer to actual google summer of code coding period which is in the end of the month. Let's aim, for landing it next week.

yamaguchi updated this revision to Diff 99390.May 17 2017, 9:14 PM
yamaguchi marked 5 inline comments as done.

Update diff.

yamaguchi marked an inline comment as done.May 17 2017, 9:14 PM
ruiu accepted this revision.May 17 2017, 9:46 PM

LGTM, but you want to wait until next week for Vassil.

I think you made a good start for your GSoC project, Yuka. Great work!

This revision is now accepted and ready to land.May 17 2017, 9:46 PM
This revision was automatically updated to reflect the committed changes.