This is an archive of the discontinued LLVM Phabricator instance.

[GSoC] Flag value completion for clang
ClosedPublic

Authored by yamaguchi on May 21 2017, 1:18 AM.

Details

Summary

This is 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.

In this patch, Options.td was mainly changed in order to add value class in Options.inc. It adds initial support for auto-completing flag values in bash. The general idea is to make Options.td aware of all possible values for all flags, and then generate a lookup data structure in the clang driver.

From the bash side we call clang with --autocomplete like this clang --autocomplete=-stdlib=,l and then the clang driver returns all possible values which starts with "l" in -stdlib's Values class.

Event Timeline

yamaguchi created this revision.May 21 2017, 1:18 AM
yamaguchi updated this revision to Diff 99688.May 21 2017, 1:35 AM

Update diff using arc.

teemperor edited edge metadata.May 22 2017, 5:25 AM

Good progress and good catch with also checking lld :)

clang/lib/Driver/Driver.cpp
1233

Split, S and V aren't that meaningful names, which makes the code is a bit hard to follow. Maybe replace it with something like ListOfPassedFlags, PassedFlags and SuggestedCompletions. Also please add some comment why you're suddenly splitting that string by ",". E.g. give an example of what a possible flag value is like -Xclang,-std= and that this is the format you expect to receive by the shell.

lld/COFF/DriverUtils.cpp
659–660

Minor thing: Please make sure the trailing '\' are still aligned after your change. Here and on line 665.

llvm/include/llvm/Option/OptParser.td
95

'Value' -> 'Values' as it contains multiple values.

Is there a reason why were not using list<string> as a type (like with the prefixes)? It seems the only downside of using list<string> is that the generated Options.inc is handling this in a bit funky way, but I think we can live with this.

llvm/include/llvm/Option/OptTable.h
117

Please add documentation :) Also findByValue is not saying that much. Maybe something like suggestValueCompletions.

llvm/lib/Option/OptTable.cpp
194

Ret -> Result.

198

Hmm, will this work with "--" flags? Also, same as above with naming S and C.

206

Please add brackets around the for body. the if is fine, but the for is not one line (even though it works, so it looks a bit out of place.

211

The whole function deserves some comments on why you do the things the way you do. Don't hesitate to give examples and describe the ideas behind all the checks/splitting/merging strings.

Especially the first three if checks in this function are not obvious why they are here.

yamaguchi updated this revision to Diff 99931.May 23 2017, 9:26 AM
yamaguchi marked 6 inline comments as done.

Update diff.

yamaguchi added inline comments.May 23 2017, 9:36 AM
llvm/lib/Option/OptTable.cpp
198

@teemperor
Do you think we should add some sample value completions to flags start with "--" (such as --target) and flags which don't contain "=" (such as -mllvm <value>) ?
This part of code is rather ad hoc right now for -stdlib.

v.g.vassilev added inline comments.May 29 2017, 6:38 AM
clang/include/clang/Driver/Options.h
42–44

Value seems too generic, shall we add ARGVALUE instead?

clang/include/clang/Driver/Options.td
2204

Values seems too generic, can we use ArgValues instead?

clang/utils/bash-autocomplete.sh
6–7

Do the changes in bash-autocomplete.sh depend on argument value completion? Shouldn't that go in a separate review?

llvm/include/llvm/Option/OptTable.h
117

We need three backslashes (///) to enable doxygen.

yamaguchi added inline comments.May 30 2017, 11:19 AM
clang/utils/bash-autocomplete.sh
6–7

Bash parses foo=bar to [foo, =, bar].
So it is somehow necessary to handle multiple argument in order to do value completion.
Eg. when "-std=foo" was given, bash parses it to ["-std", "=", "foo"], but we want all of the array, not just one element of it.

yamaguchi updated this revision to Diff 100798.May 30 2017, 4:52 PM
yamaguchi marked 3 inline comments as done.

Update patch. Changed Values to ArgValues.

yamaguchi edited the summary of this revision. (Show Details)Jun 5 2017, 8:42 AM
ruiu edited edge metadata.Jun 14 2017, 11:28 AM

Overall looking good. Good work! A few minor comments.

clang/include/clang/Driver/Options.td
2204

I'd keep it as Values, as everything is essentially related to command line arguments, and Args seems redundant. For example, we didn't name ArgFlags but just Flags, and HelpText instead of ArgHelpText, etc.

clang/lib/Driver/Driver.cpp
1232

nit: insert a blank line before a meaningful code block so that code doesn't look too dense.

1233

make a list of flag values.

1234

You are using PassedFlags only once. Do you want to replace it with StringRef(A->getValue()).split(...)?

clang/test/Driver/autocomplete.c
7

Why do you want to pass the arguments in the reverse order?

v.g.vassilev added inline comments.Jun 16 2017, 9:11 AM
clang/include/clang/Driver/Options.td
2204

My reasoning for asking this is that I wanted to hint about the relationship between the values (as this is a very broad term) and arguments. I'd read ArgValues as possible values for an argument without having to dig into context.

That said, I don't think switching back to Values is a significant issue, if @ruiu feels strongly about it , please follow his suggestion and land the patch.

clang/lib/Driver/Driver.cpp
1234

PassedFlags looks more readable to casual readers like me ;)

yamaguchi updated this revision to Diff 102944.Jun 17 2017, 1:20 PM
yamaguchi marked 3 inline comments as done.

Update patch and add support for clang -stdlib=[tab] case.
In this case we expect to see all possible values.
(Eg. libc++ libstdc++ platform for clang -stdlib=[tab]).

yamaguchi added inline comments.Jun 17 2017, 1:21 PM
clang/test/Driver/autocomplete.c
7

In the reverse order, current flag is always exists at first, so I thought it is more simple. (Eg. clang -fsyntax-only -o foo -std=c will be passed as c,=,-std,foo,-i,-fsyntax-only)

yamaguchi marked 2 inline comments as done.Jun 17 2017, 1:23 PM
yamaguchi added inline comments.
llvm/lib/Option/OptTable.cpp
198

I had add support for this in this update as well, thanks.

yamaguchi edited the summary of this revision. (Show Details)Jun 17 2017, 1:34 PM
ruiu added a comment.Jun 17 2017, 1:37 PM

As to the order of Command variable contents, I think I'm not convinced. You can access the last element as Command[Command.size() - 1] (or maybe Command.back()), no? It is slightly awkward than Command[0], but that's not horribly hard to handle, and passing arguments in the reverse order seems more counter-intuitive to me.

clang/include/clang/Driver/Options.td
2204

I think I feel fairly strongly prefer "Value" over "ArgValue". If this is a one-time name, I agree that we should use "ArgValue", but this is not the case. We are going to add bunch of "Values" to options that take arguments, and I expect that the verbose name will start feeling too verbose. So please switch back to "Values".

clang/utils/bash-autocomplete.sh
7–32

Don't you want to make it local?

8

If you don't write do on the same line as for, you don't need to write ; at end of a line. I.e. write it in either

for ...
do
   command...
done

or

for ...; do
  command...
done

This is because you need a newline between for and do, and ; works as a newline.

10

nit: you don't need double double-quotes. Instead, `"$arg${COMP_WORDS[$i]}," should just work.

15–16

Alternatively, COMPRELY=( $( compgen -W "$flags" -- "") ) should work.

17

Use elif so that you don't have to write two fis at end. In a bash script, if ... else if is usually written as

if ...; then
elif ...; then
elif ...; then
fi

If you use else if instead of elif, and if you indent the code properly, it'll look like

if ...; then
else
  if ...; then
  fi
fi

This is why you needed two fis.

ruiu added inline comments.Jun 17 2017, 1:55 PM
clang/utils/bash-autocomplete.sh
10

On second thought, I think what you actually want to do is to concatenate the elements of the array with ,, without adding , at end of the string. For example, if COMP_WORDS contains "a", "b" and "c", you want to create "a,b,c" instead of "a,b,c," or ",a,b,c".

There are surprisingly large number of different ways of doing this, but it seems this is one of the easiest ways.

arg="$(IFS=,; echo "${COMP_WORDS[*]}")"

IFS is a special shell variable containing a word splitting words. By setting the variable, COMP_WORDS[*] is expanded to (for example) "a,b,c" instead of the default "a b c".

yamaguchi updated this revision to Diff 102946.Jun 17 2017, 1:58 PM
yamaguchi marked 5 inline comments as done.

Update patch. Rui's comment about the bash part is very reasonable.

yamaguchi marked 10 inline comments as done.

Cleaned suggestValueCompletions and bash-autocomplete.sh, and added tests.

We have passed flags and values from bash to clang like clang --autocomplete=l,=,-stdlib, but I've changed this to pass clang --autocomplte=-stdlib=,l instead. This is more simple.

Added support and test for -meabi flag.
This flag is corner case, because it doesn't take = suffix like -stdlib=.

When clang -stdlib=[tab] is pushed, we would give possible values (libstd++,..), and when clang -stdlib= [tab] is pushed, we just give files under current directory.
And when clang -meabi [tab] is pushed, we give possible values (gnu,..).

yamaguchi edited the summary of this revision. (Show Details)Jun 17 2017, 11:17 PM
yamaguchi marked an inline comment as done.

Made trivial change.

Made trivial bug fix.

Made trivial change.

Made a trivial change in optionMatches.

ruiu added a comment.Jun 18 2017, 8:53 AM

Yuka,

This is beautiful. Overall looking pretty good. Some minor stylistic comments.

clang/include/clang/Driver/Options.h
43

Since you are now using Values as a variable name, you want to rename ARGVALUE VALUES.

clang/include/clang/Driver/Options.td
1709–1710

You wrote Values before HelpText here but in the opposite order for stdlib_EQ. This is not a strict rule or anything, but it is better to be consistent in the option order.

clang/lib/Driver/Driver.cpp
1233

Move this inside the first if so that it is clear that this comment does not describe the entire if and else but only the if part.

You could improve the comment: If the flag is in the form of "--autocomplete=-foo", we were requested to print out all option names that start with "-foo". For example, "--autocomplete=-fsyn" is expanded to "-fsyntax-only".

1238

Likewise: If the flag is in the form of "--autocomplete=-foo,bar", we were requested to print out all option values for "-foo" that start with "bar". For example, "--autocomplete=-stdlib=,l" is expanded to "libc++" and "libstdc++".

clang/utils/bash-autocomplete.sh
7–32

It is better to describe what you are doing here and why. How about this:

bash autocompletion always separates '=' as a token even if there's no space before/after it. On the other hand, '=' is just a regular character for clang options that contain '='. For example, "-stdlib=" is defined as is, instead of "-stdlib" and "=". So, we need to partially undo bash tokenization here for impedance matching.

llvm/lib/Option/OptTable.cpp
189

returns -> Returns

yamaguchi updated this revision to Diff 102981.Jun 18 2017, 4:02 PM
yamaguchi marked 6 inline comments as done.

Update code comment according to Rui's comment.

yamaguchi updated this revision to Diff 102982.Jun 18 2017, 5:20 PM

Add support and test for -cl-std=,-fno-sanitize-coverage=,-ffp-contract=,-flto=,-fveclib=,-fshow-overloads=,-fvisibility=,-mfloat-abi=,-mthread-model.

v.g.vassilev accepted this revision.Jun 19 2017, 2:40 AM

LGTM. My comments can be addressed in a separate patch.

clang/include/clang/Driver/Options.td
496

I think we should have a way to express that the values are case insensitive, instead of enumerating all possible combinations.

1341

We should be able to express which value is the default value if nothing is specified. A non-very readable solution would be to put the default always as the first value.

This revision is now accepted and ready to land.Jun 19 2017, 2:40 AM
ruiu accepted this revision.Jun 19 2017, 6:16 AM

LGTM

ruiu added inline comments.Jun 19 2017, 9:59 AM
clang/include/clang/Driver/Options.td
496

Looks like this is not really a case-insensitive option, as it does not allow "cL2.0" for example. The enumerated values are exact list of values that the option accepts.