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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
