Re: [PATCH 3/3] Come up with new --completion option.

[David Malcolm <dmalcolm at redhat dot com>]

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