[PATCH] D33383: [GSoC] Flag value completion for clang

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

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.



================
Comment at: clang/include/clang/Driver/Options.td:2198
 def stdlib_EQ : Joined<["-", "--"], "stdlib=">, Flags<[CC1Option]>,
-  HelpText<"C++ standard library to use">;
+  HelpText<"C++ standard library to use">, Values<"libc++,libstdc++,platform">;
 def sub__library : JoinedOrSeparate<["-"], "sub_library">;
----------------
v.g.vassilev wrote:
> ruiu wrote:
> > v.g.vassilev wrote:
> > > `Values` seems too generic, can we use `ArgValues` instead?
> > 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.
> 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.
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".


================
Comment at: clang/utils/bash-autocomplete.sh:7
 
-  flags=$( clang --autocomplete="$cur" )
-  if [[ "$flags" == "" || "$cur" == "" ]]; then
+  arg=""
+  for (( i = $cword; i >= 1; i-- )) ;
----------------
Don't you want to make it `local`?


================
Comment at: clang/utils/bash-autocomplete.sh:8
+  arg=""
+  for (( i = $cword; i >= 1; i-- )) ;
+  do
----------------
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.


================
Comment at: clang/utils/bash-autocomplete.sh:10
+  do
+    arg="$arg""${COMP_WORDS[$i]},"
+  done
----------------
nit: you don't need double double-quotes. Instead, `"$arg${COMP_WORDS[$i]}," should just work.


================
Comment at: clang/utils/bash-autocomplete.sh:15-16
+  if [[ "$cur" == "=" ]]; then
+    cur=""
+    COMPREPLY=( $( compgen -W "$flags" -- "$cur") )
+  else if [[ "$flags" == "" || "$cur" == "" ]]; then
----------------
Alternatively, `COMPRELY=( $( compgen -W "$flags" -- "") )` should work.


================
Comment at: clang/utils/bash-autocomplete.sh:17
+    COMPREPLY=( $( compgen -W "$flags" -- "$cur") )
+  else if [[ "$flags" == "" || "$cur" == "" ]]; then
     _filedir
----------------
Use `elif` so that you don't have to write two `fi`s 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 `fi`s.


https://reviews.llvm.org/D33383