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

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

Yuka,

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



================
Comment at: clang/include/clang/Driver/Options.h:43
+#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
+               HELPTEXT, METAVAR, ARGVALUE)                                    \
+  OPT_##ID,
----------------
Since you are now using `Values` as a variable name, you want to rename ARGVALUE VALUES.


================
Comment at: clang/include/clang/Driver/Options.td:1709-1710
   HelpText<"The thread model to use, e.g. posix, single (posix by default)">;
-def meabi : Separate<["-"], "meabi">, Group<m_Group>, Flags<[CC1Option]>,
+def meabi : Separate<["-"], "meabi">, Group<m_Group>, Flags<[CC1Option]>, Values<"default,4,5,gnu">,
   HelpText<"Set EABI type, e.g. 4, 5 or gnu (default depends on triple)">;
 
----------------
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.


================
Comment at: clang/lib/Driver/Driver.cpp:1233
+
+    // When the flag was passed like "--autocomplete=-fsyn"
+    // we want to autocomplete "-fsyn" as "-fsyntax-only"
----------------
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".


================
Comment at: clang/lib/Driver/Driver.cpp:1238
+    } else {
+      // When flags were passed like "--autocomplete=-stdlib=,l"
+      // we want to autocomplete appropriate value which starts with "l"
----------------
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++".


================
Comment at: clang/utils/bash-autocomplete.sh:7
 
-  flags=$( clang --autocomplete="$cur" )
-  if [[ "$flags" == "" || "$cur" == "" ]]; then
+  local w1="${COMP_WORDS[$cword - 1]}"
+  local w2="${COMP_WORDS[$cword - 2]}"
----------------
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.


================
Comment at: llvm/lib/Option/OptTable.cpp:189
 
+// returns true if one of the Prefixes + In.Names matches Option
+static bool optionMatches(const OptTable::Info &In, StringRef Option) {
----------------
returns -> Returns


https://reviews.llvm.org/D33383