[PATCH 3/3] Come up with new --completion option.
David Malcolm
dmalcolm@redhat.com
Wed Jun 20 15:27:00 GMT 2018
On Mon, 2018-05-14 at 14:51 +0200, Martin Liška wrote:
> Main part where I still need to write ChangeLog file and
> gcc.sh needs to be moved to bash-completions project.
>
> Martin
As before, I'm not an official reviewer for it, but it touches code
that I wrote, so here goes.
Overall looks good to me, but I have some nitpicks:
(needs a ChangeLog)
[...snip gcc.sh change; I don't feel qualified to comment...]
[...snip sane-looking changes to common.opt and gcc.c. */
diff --git a/gcc/opt-suggestions.c b/gcc/opt-suggestions.c
index e322fcd91c5..2da094a5960 100644
--- a/gcc/opt-suggestions.c
+++ b/gcc/opt-suggestions.c
@@ -28,18 +28,6 @@ along with GCC; see the file COPYING3. If not see
#include "opt-suggestions.h"
#include "selftest.h"
-option_proposer::~option_proposer ()
-{
- if (m_option_suggestions)
- {
- int i;
- char *str;
- FOR_EACH_VEC_ELT (*m_option_suggestions, i, str)
- free (str);
- delete m_option_suggestions;
- }
-}
Why is this dtor going away? If I'm reading the patch correctly,
option_proposer still "owns" m_option_suggestions.
What happens if you run "make selftest-valgrind" ? (my guess is some
new memory leaks)
+void
+option_proposer::get_completions (const char *option_prefix,
+ auto_string_vec &results)
Missing leading comment. Maybe something like:
/* Populate RESULTS with valid completions of options that begin
with OPTION_PREFIX. */
or somesuch.
+{
+ /* Bail out for an invalid input. */
+ if (option_prefix == NULL || option_prefix[0] == '\0')
+ return;
+
+ /* Option suggestions are built without first leading dash character. */
+ if (option_prefix[0] == '-')
+ option_prefix++;
+
+ size_t length = strlen (option_prefix);
+
+ /* Handle parameters. */
+ const char *prefix = "-param";
+ if (length >= strlen (prefix)
+ && strstr (option_prefix, prefix) == option_prefix)
Maybe reword that leading comment to
/* Handle OPTION_PREFIX starting with "-param". */
(or somesuch) to clarify the intent?
[...snip...]
+void
+option_proposer::suggest_completion (const char *option_prefix)
+{
Missing leading comment. Maybe something like:
/* Print on stdout a list of valid options that begin with OPTION_PREFIX,
one per line, suitable for use by Bash completion.
Implementation of the "-completion=" option. */
or somesuch.
[...snip...]
+void
+option_proposer::find_param_completions (const char separator,
+ const char *option_prefix,
+ auto_string_vec &results)
Maybe rename "option_prefix" to "param_prefix"? I believe it's now
the prefix of the param name, rather than the option.
Missing leading comment. Maybe something like:
/* Populate RESULTS with bash-completions options for all parameters
that begin with PARAM_PREFIX, using SEPARATOR.
For example, if PARAM_PREFIX is "max-" and SEPARATOR is "=" then
strings of the form:
"--param=max-unrolled-insns"
"--param=max-early-inliner-iterations"
will be added to RESULTS. */
(did I get that right?)
+{
+ char separator_str[] {separator, '\0'};
Is the above initialization valid C++98, or is it a C++11-ism?
+ size_t length = strlen (option_prefix);
+ for (unsigned i = 0; i < get_num_compiler_params (); ++i)
+ {
+ const char *candidate = compiler_params[i].option;
+ if (strlen (candidate) >= length
+ && strstr (candidate, option_prefix) == candidate)
+ results.safe_push (concat ("--param", separator_str, candidate, NULL));
+ }
+}
+
+#if CHECKING_P
+
+namespace selftest {
+
+/* Verify that PROPOSER generates sane auto-completion suggestions
+ for OPTION_PREFIX. */
+static void
+verify_autocompletions (option_proposer &proposer, const char *option_prefix)
+{
+ auto_string_vec suggestions;
+ proposer.get_completions (option_prefix, suggestions);
Maybe a comment here to specify:
/* There must be at least one suggestion, and every suggestion must
indeed begin with OPTION_PREFIX. */
+ ASSERT_GT (suggestions.length (), 0);
+
+ for (unsigned i = 0; i < suggestions.length (); i++)
+ ASSERT_STR_STARTSWITH (suggestions[i], option_prefix);
+}
[...snip...]
diff --git a/gcc/opt-suggestions.h b/gcc/opt-suggestions.h
index 5e3ee9e2671..d0c32af7e5c 100644
--- a/gcc/opt-suggestions.h
+++ b/gcc/opt-suggestions.h
@@ -33,9 +33,6 @@ public:
option_proposer (): m_option_suggestions (NULL)
{}
- /* Default destructor. */
- ~option_proposer ();
Again, why does this dtor go away?
+ /* Find parameter completions for --param format with SEPARATOR.
+ Again, save the completions into results. */
+ void find_param_completions (const char separator, const char *option_prefix,
+ auto_string_vec &results);
If we're renaming "option_prefix" to "param_prefix", please do so here as well.
private:
/* Cache with all suggestions. */
auto_vec <char *> *m_option_suggestions;
[...snip...]
+/* Evaluate STRING and PREFIX and use strstr to determine if STRING
+ starts with PREFIX.
+ ::selftest::pass if starts.
+ ::selftest::fail if does not start. */
"strstr" seems like an implementation detail to me (or am I missing
something here?). Maybe reword to:
/* Evaluate STRING and PREFIX and determine if STRING starts with PREFIX.
::selftest::pass if STRING does start with PREFIX.
::selftest::fail if does not, or either is NULL. */
Thanks for working on this; the rest looks good to me (though as I
said, I'm not officially a reviewer for this).
Hope this is constructive
Dave
More information about the Gcc-patches
mailing list