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

Main part where I still need to write ChangeLog file and
gcc.sh needs to be moved to bash-completions project.

Martin

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

On 06/20/2018 05:27 PM, David Malcolm wrote:
> 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)

Added.

> 
> [...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)

Fixed that, should not be removed.

>  
> +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?

Thanks, done.

> 
> [...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.

Likewise.

> 
> [...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.

Makes sense.

> 
> Missing leading comment.  Maybe something like:
> 
> /* Populate RESULTS with bash-completions options for all parameters
>    that begin with PARAM_PREFIX, using SEPARATOR.

It's in header file, thus I copied that.

> 
>    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?)

Yes.

> 
> +{
> +  char separator_str[] {separator, '\0'};
> 
> Is the above initialization valid C++98, or is it a C++11-ism?

You are right. I fixed that and 2 more occurrences of the same
issue.

> 
> +  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.  */

Yes!

> 
> +  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?

Fixed.

> 
>  
> +  /* 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.

Done.

> 
>  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).

Thanks for the review.

> 
> Hope this is constructive

Yes, very.

Martin

> Dave
>
From c9722d3416f440a053cedcb46689917f7ff21f0c Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Mon, 14 May 2018 11:49:52 +0200
Subject: [PATCH] Come up with new --completion option.

gcc/ChangeLog:

2018-06-22  Martin Liska  <mliska@suse.cz>

	* common.opt: Introduce -completion option.
	* gcc.c (driver_handle_option): Handle it.
	(driver::main): Print completions if completion
        is set.
	* opt-suggestions.c (option_proposer::get_completions):
        New function.
	(option_proposer::suggest_completion): Likewise.
	(option_proposer::find_param_completions): Likewise.
	(verify_autocompletions): Likewise.
	(test_completion_valid_options): Likewise.
	(test_completion_valid_params): Likewise.
	(in_completion_p): Likewise.
	(empty_completion_p): Likewise.
	(test_completion_partial_match): Likewise.
	(test_completion_garbage): Likewise.
	(opt_proposer_c_tests): Likewise.
	* opt-suggestions.h: Declare new functions.
	* opts.c (common_handle_option): Handle OPT__completion_.
	* selftest-run-tests.c (selftest::run_tests): Add
        opt_proposer_c_tests.
	* selftest.c (assert_str_startswith): New.
	* selftest.h (assert_str_startswith): Likewise.
	(opt_proposer_c_tests): New.
	(ASSERT_STR_STARTSWITH): Likewise.
---
 gcc.sh                   |  51 +++++++
 gcc/common.opt           |   4 +
 gcc/gcc.c                |  15 ++
 gcc/opt-suggestions.c    | 293 +++++++++++++++++++++++++++++++++++++++
 gcc/opt-suggestions.h    |  15 ++
 gcc/opts.c               |   3 +
 gcc/selftest-run-tests.c |   1 +
 gcc/selftest.c           |  33 +++++
 gcc/selftest.h           |  20 +++
 9 files changed, 435 insertions(+)
 create mode 100644 gcc.sh

diff --git a/gcc.sh b/gcc.sh
new file mode 100644
index 00000000000..06b16b3152b
--- /dev/null
+++ b/gcc.sh
@@ -0,0 +1,51 @@
+# Please add "source /path/to/bash-autocomplete.sh" to your .bashrc to use this.
+
+log()
+{
+  echo $1 >> /tmp/bash-completion.log
+}
+
+_gcc()
+{
+    local cur prev prev2 words cword argument prefix
+    _init_completion || return
+    _expand || return
+
+    # extract also for situations like: -fsanitize=add
+    if [[ $cword > 2 ]]; then
+      prev2="${COMP_WORDS[$cword - 2]}"
+    fi
+
+    log "cur: '$cur', prev: '$prev': prev2: '$prev2' cword: '$cword'"
+
+    # sample: -fsan
+    if [[ "$cur" == -* ]]; then
+      argument=$cur
+    # sample: -fsanitize=
+    elif [[ "$cur" == "=" && $prev == -* ]]; then
+      argument=$prev$cur
+      prefix=$prev$cur
+    # sample: -fsanitize=add
+    elif [[ "$prev" == "=" && $prev2 == -* ]]; then
+      argument=$prev2$prev$cur
+      prefix=$prev2$prev
+    # sample: --param lto-
+    elif [[ "$prev" == "--param" ]]; then
+      argument="$prev $cur"
+      prefix="$prev "
+    fi
+
+    log  "argument: '$argument', prefix: '$prefix'"
+
+    if [[ "$argument" == "" ]]; then
+      _filedir
+    else
+      # In situation like '-fsanitize=add' $cur is equal to last token.
+      # Thus we need to strip the beginning of suggested option.
+      flags=$( gcc --completion="$argument" 2>/dev/null | sed "s/^$prefix//")
+      log "compgen: $flags"
+      [[ "${flags: -1}" == '=' ]] && compopt -o nospace 2> /dev/null
+      COMPREPLY=( $( compgen -W "$flags" -- "") )
+    fi
+}
+complete -F _gcc gcc
diff --git a/gcc/common.opt b/gcc/common.opt
index 0d1445b8529..5a50bc27710 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -255,6 +255,10 @@ Driver Alias(S)
 -compile
 Driver Alias(c)
 
+-completion=
+Common Driver Joined Undocumented
+Provide bash completion for options starting with provided string.
+
 -coverage
 Driver Alias(coverage)
 
diff --git a/gcc/gcc.c b/gcc/gcc.c
index dda1fd35398..9e5b153bc53 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -221,6 +221,10 @@ static int print_help_list;
 
 static int print_version;
 
+/* Flag that stores string value for which we provide bash completion.  */
+
+static const char *completion = NULL;
+
 /* Flag indicating whether we should ONLY print the command and
    arguments (like verbose_flag) without executing the command.
    Displayed arguments are quoted so that the generated command
@@ -3890,6 +3894,11 @@ driver_handle_option (struct gcc_options *opts,
       add_linker_option ("--version", strlen ("--version"));
       break;
 
+    case OPT__completion_:
+      validated = true;
+      completion = decoded->arg;
+      break;
+
     case OPT__help:
       print_help_list = 1;
 
@@ -7300,6 +7309,12 @@ driver::main (int argc, char **argv)
   maybe_putenv_OFFLOAD_TARGETS ();
   handle_unrecognized_options ();
 
+  if (completion)
+    {
+      m_option_proposer.suggest_completion (completion);
+      return 0;
+    }
+
   if (!maybe_print_and_exit ())
     return 0;
 
diff --git a/gcc/opt-suggestions.c b/gcc/opt-suggestions.c
index 90ab80e627d..894eea5f37c 100644
--- a/gcc/opt-suggestions.c
+++ b/gcc/opt-suggestions.c
@@ -47,6 +47,66 @@ option_proposer::suggest_option (const char *bad_opt)
      (auto_vec <const char *> *) m_option_suggestions);
 }
 
+/* Populate RESULTS with valid completions of options that begin
+   with OPTION_PREFIX.  */
+
+void
+option_proposer::get_completions (const char *option_prefix,
+				  auto_string_vec &results)
+{
+  /* 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 OPTION_PREFIX starting with "-param".  */
+  const char *prefix = "-param";
+  if (length >= strlen (prefix)
+      && strstr (option_prefix, prefix) == option_prefix)
+    {
+      /* We support both '-param-xyz=123' and '-param xyz=123' */
+      option_prefix += strlen (prefix);
+      char separator = option_prefix[0];
+      option_prefix++;
+      if (separator == ' ' || separator == '=')
+	find_param_completions (separator, option_prefix, results);
+    }
+  else
+    {
+      /* Lazily populate m_option_suggestions.  */
+      if (!m_option_suggestions)
+	build_option_suggestions ();
+      gcc_assert (m_option_suggestions);
+
+      for (unsigned i = 0; i < m_option_suggestions->length (); i++)
+	{
+	  char *candidate = (*m_option_suggestions)[i];
+	  if (strlen (candidate) >= length
+	      && strstr (candidate, option_prefix) == candidate)
+	    results.safe_push (concat ("-", candidate, NULL));
+	}
+    }
+}
+
+/* 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.  */
+
+void
+option_proposer::suggest_completion (const char *option_prefix)
+{
+  auto_string_vec results;
+  get_completions (option_prefix, results);
+  for (unsigned i = 0; i < results.length (); i++)
+    printf ("%s\n", results[i]);
+}
+
 void
 option_proposer::build_option_suggestions (void)
 {
@@ -120,3 +180,236 @@ option_proposer::build_option_suggestions (void)
 	}
     }
 }
+
+/* Find parameter completions for --param format with SEPARATOR.
+   Again, save the completions into results.  */
+
+void
+option_proposer::find_param_completions (const char separator,
+					 const char *param_prefix,
+					 auto_string_vec &results)
+{
+  char separator_str[] = {separator, '\0'};
+  size_t length = strlen (param_prefix);
+  for (unsigned i = 0; i < get_num_compiler_params (); ++i)
+    {
+      const char *candidate = compiler_params[i].option;
+      if (strlen (candidate) >= length
+	  && strstr (candidate, param_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);
+
+  /* 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);
+}
+
+/* Verify that valid options are auto-completed correctly.  */
+
+static void
+test_completion_valid_options (option_proposer &proposer)
+{
+  const char *option_prefixes[] =
+  {
+    "-fno-var-tracking-assignments-toggle",
+    "-fpredictive-commoning",
+    "--param=stack-clash-protection-guard-size",
+    "--param=max-predicted-iterations",
+    "-ftree-loop-distribute-patterns",
+    "-fno-var-tracking",
+    "-Walloc-zero",
+    "--param=ipa-cp-value-list-size",
+    "-Wsync-nand",
+    "-Wno-attributes",
+    "--param=tracer-dynamic-coverage-feedback",
+    "-Wno-format-contains-nul",
+    "-Wnamespaces",
+    "-fisolate-erroneous-paths-attribute",
+    "-Wno-underflow",
+    "-Wtarget-lifetime",
+    "--param=asan-globals",
+    "-Wno-empty-body",
+    "-Wno-odr",
+    "-Wformat-zero-length",
+    "-Wstringop-truncation",
+    "-fno-ipa-vrp",
+    "-fmath-errno",
+    "-Warray-temporaries",
+    "-Wno-unused-label",
+    "-Wreturn-local-addr",
+    "--param=sms-dfa-history",
+    "--param=asan-instrument-reads",
+    "-Wreturn-type",
+    "-Wc++17-compat",
+    "-Wno-effc++",
+    "--param=max-fields-for-field-sensitive",
+    "-fisolate-erroneous-paths-dereference",
+    "-fno-defer-pop",
+    "-Wcast-align=strict",
+    "-foptimize-strlen",
+    "-Wpacked-not-aligned",
+    "-funroll-loops",
+    "-fif-conversion2",
+    "-Wdesignated-init",
+    "--param=max-iterations-computation-cost",
+    "-Wmultiple-inheritance",
+    "-fno-sel-sched-reschedule-pipelined",
+    "-Wassign-intercept",
+    "-Wno-format-security",
+    "-fno-sched-stalled-insns",
+    "-fbtr-bb-exclusive",
+    "-fno-tree-tail-merge",
+    "-Wlong-long",
+    "-Wno-unused-but-set-parameter",
+    NULL
+  };
+
+  for (const char **ptr = option_prefixes; *ptr != NULL; ptr++)
+    verify_autocompletions (proposer, *ptr);
+}
+
+/* Verify that valid parameters are auto-completed correctly,
+   both with the "--param=PARAM" form and the "--param PARAM" form.  */
+
+static void
+test_completion_valid_params (option_proposer &proposer)
+{
+  const char *option_prefixes[] =
+  {
+    "--param=sched-state-edge-prob-cutoff",
+    "--param=iv-consider-all-candidates-bound",
+    "--param=align-threshold",
+    "--param=prefetch-min-insn-to-mem-ratio",
+    "--param=max-unrolled-insns",
+    "--param=max-early-inliner-iterations",
+    "--param=max-vartrack-reverse-op-size",
+    "--param=ipa-cp-loop-hint-bonus",
+    "--param=tracer-min-branch-ratio",
+    "--param=graphite-max-arrays-per-scop",
+    "--param=sink-frequency-threshold",
+    "--param=max-cse-path-length",
+    "--param=sra-max-scalarization-size-Osize",
+    "--param=prefetch-latency",
+    "--param=dse-max-object-size",
+    "--param=asan-globals",
+    "--param=max-vartrack-size",
+    "--param=case-values-threshold",
+    "--param=max-slsr-cand-scan",
+    "--param=min-insn-to-prefetch-ratio",
+    "--param=tracer-min-branch-probability",
+    "--param sink-frequency-threshold",
+    "--param max-cse-path-length",
+    "--param sra-max-scalarization-size-Osize",
+    "--param prefetch-latency",
+    "--param dse-max-object-size",
+    "--param asan-globals",
+    "--param max-vartrack-size",
+    NULL
+  };
+
+  for (const char **ptr = option_prefixes; *ptr != NULL; ptr++)
+    verify_autocompletions (proposer, *ptr);
+}
+
+/* Return true when EXPECTED is one of completions for OPTION_PREFIX string.  */
+
+static bool
+in_completion_p (option_proposer &proposer, const char *option_prefix,
+		 const char *expected)
+{
+  auto_string_vec suggestions;
+  proposer.get_completions (option_prefix, suggestions);
+
+  for (unsigned i = 0; i < suggestions.length (); i++)
+    {
+      char *r = suggestions[i];
+      if (strcmp (r, expected) == 0)
+	return true;
+    }
+
+  return false;
+}
+
+/* Return true when PROPOSER does not find any partial completion
+   for OPTION_PREFIX.  */
+
+static bool
+empty_completion_p (option_proposer &proposer, const char *option_prefix)
+{
+  auto_string_vec suggestions;
+  proposer.get_completions (option_prefix, suggestions);
+  return suggestions.is_empty ();
+}
+
+/* Verify autocompletions of partially-complete options.  */
+
+static void
+test_completion_partial_match (option_proposer &proposer)
+{
+  ASSERT_TRUE (in_completion_p (proposer, "-fsani", "-fsanitize=address"));
+  ASSERT_TRUE (in_completion_p (proposer, "-fsani",
+				"-fsanitize-address-use-after-scope"));
+  ASSERT_TRUE (in_completion_p (proposer, "-fipa-icf", "-fipa-icf-functions"));
+  ASSERT_TRUE (in_completion_p (proposer, "-fipa-icf", "-fipa-icf"));
+  ASSERT_TRUE (in_completion_p (proposer, "--param=",
+				"--param=max-vartrack-reverse-op-size"));
+  ASSERT_TRUE (in_completion_p (proposer, "--param ",
+				"--param max-vartrack-reverse-op-size"));
+
+  ASSERT_FALSE (in_completion_p (proposer, "-fipa-icf", "-fipa"));
+  ASSERT_FALSE (in_completion_p (proposer, "-fipa-icf-functions", "-fipa-icf"));
+
+  ASSERT_FALSE (empty_completion_p (proposer, "-"));
+  ASSERT_FALSE (empty_completion_p (proposer, "-fipa"));
+  ASSERT_FALSE (empty_completion_p (proposer, "--par"));
+}
+
+/* Verify that autocompletion does not return any match for garbage inputs.  */
+
+static void
+test_completion_garbage (option_proposer &proposer)
+{
+  ASSERT_TRUE (empty_completion_p (proposer, NULL));
+  ASSERT_TRUE (empty_completion_p (proposer, ""));
+  ASSERT_TRUE (empty_completion_p (proposer, "- "));
+  ASSERT_TRUE (empty_completion_p (proposer, "123456789"));
+  ASSERT_TRUE (empty_completion_p (proposer, "---------"));
+  ASSERT_TRUE (empty_completion_p (proposer, "#########"));
+  ASSERT_TRUE (empty_completion_p (proposer, "- - - - - -"));
+  ASSERT_TRUE (empty_completion_p (proposer, "-fsanitize=address2"));
+}
+
+/* Run all of the selftests within this file.  */
+
+void
+opt_proposer_c_tests ()
+{
+  option_proposer proposer;
+
+  test_completion_valid_options (proposer);
+  test_completion_valid_params (proposer);
+  test_completion_partial_match (proposer);
+  test_completion_garbage (proposer);
+}
+
+} // namespace selftest
+
+#endif /* #if CHECKING_P */
diff --git a/gcc/opt-suggestions.h b/gcc/opt-suggestions.h
index ccd4e45a474..222bafa12cd 100644
--- a/gcc/opt-suggestions.h
+++ b/gcc/opt-suggestions.h
@@ -45,12 +45,27 @@ public:
      The returned string is owned by the option_proposer instance.  */
   const char *suggest_option (const char *bad_opt);
 
+  /* 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.  */
+  void suggest_completion (const char *option_prefix);
+
+  /* Populate RESULTS with valid completions of options that begin
+     with OPTION_PREFIX.  */
+  void get_completions (const char *option_prefix, auto_string_vec &results);
+
 private:
   /* Helper function for option_proposer::suggest_option.  Populate
      m_option_suggestions with candidate strings for misspelled options.
      The strings will be freed by the option_proposer's dtor.  */
   void build_option_suggestions ();
 
+  /* Find parameter completions for --param format with SEPARATOR.
+     Again, save the completions into results.  */
+  void find_param_completions (const char separator, const char *param_prefix,
+			       auto_string_vec &results);
+
 private:
   /* Cache with all suggestions.  */
   auto_string_vec *m_option_suggestions;
diff --git a/gcc/opts.c b/gcc/opts.c
index 33efcc0d6e7..ed102c05c22 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -1982,6 +1982,9 @@ common_handle_option (struct gcc_options *opts,
       opts->x_exit_after_options = true;
       break;
 
+    case OPT__completion_:
+      break;
+
     case OPT_fsanitize_:
       opts->x_flag_sanitize
 	= parse_sanitizer_options (arg, loc, code,
diff --git a/gcc/selftest-run-tests.c b/gcc/selftest-run-tests.c
index fe221ff8946..0b45c479192 100644
--- a/gcc/selftest-run-tests.c
+++ b/gcc/selftest-run-tests.c
@@ -70,6 +70,7 @@ selftest::run_tests ()
   fibonacci_heap_c_tests ();
   typed_splay_tree_c_tests ();
   unique_ptr_tests_cc_tests ();
+  opt_proposer_c_tests ();
 
   /* Mid-level data structures.  */
   input_c_tests ();
diff --git a/gcc/selftest.c b/gcc/selftest.c
index 74adc63e65c..78ae778ca14 100644
--- a/gcc/selftest.c
+++ b/gcc/selftest.c
@@ -125,6 +125,39 @@ assert_str_contains (const location &loc,
 	 desc_haystack, desc_needle, val_haystack, val_needle);
 }
 
+/* Implementation detail of ASSERT_STR_STARTSWITH.
+   Use strstr to determine if val_haystack starts with val_needle.
+   ::selftest::pass if it starts.
+   ::selftest::fail if it does not start.  */
+
+void
+assert_str_startswith (const location &loc,
+		       const char *desc_str,
+		       const char *desc_prefix,
+		       const char *val_str,
+		       const char *val_prefix)
+{
+  /* If val_str is NULL, fail with a custom error message.  */
+  if (val_str == NULL)
+    fail_formatted (loc, "ASSERT_STR_STARTSWITH (%s, %s) str=NULL",
+		    desc_str, desc_prefix);
+
+  /* If val_prefix is NULL, fail with a custom error message.  */
+  if (val_prefix == NULL)
+    fail_formatted (loc,
+		    "ASSERT_STR_STARTSWITH (%s, %s) str=\"%s\" prefix=NULL",
+		    desc_str, desc_prefix, val_str);
+
+  const char *test = strstr (val_str, val_prefix);
+  if (test == val_str)
+    pass (loc, "ASSERT_STR_STARTSWITH");
+  else
+    fail_formatted
+	(loc, "ASSERT_STR_STARTSWITH (%s, %s) str=\"%s\" prefix=\"%s\"",
+	 desc_str, desc_prefix, val_str, val_prefix);
+}
+
+
 /* Constructor.  Generate a name for the file.  */
 
 named_temp_file::named_temp_file (const char *suffix)
diff --git a/gcc/selftest.h b/gcc/selftest.h
index fc47b2c9ad1..8ef034e0d84 100644
--- a/gcc/selftest.h
+++ b/gcc/selftest.h
@@ -78,6 +78,15 @@ extern void assert_str_contains (const location &loc,
 				 const char *val_haystack,
 				 const char *val_needle);
 
+/* Implementation detail of ASSERT_STR_STARTSWITH.  */
+
+extern void assert_str_startswith (const location &loc,
+				   const char *desc_str,
+				   const char *desc_prefix,
+				   const char *val_str,
+				   const char *val_prefix);
+
+
 /* A named temporary file for use in selftests.
    Usable for writing out files, and as the base class for
    temp_source_file.
@@ -216,6 +225,7 @@ extern void unique_ptr_tests_cc_tests ();
 extern void vec_c_tests ();
 extern void vec_perm_indices_c_tests ();
 extern void wide_int_cc_tests ();
+extern void opt_proposer_c_tests ();
 
 extern int num_passes;
 
@@ -401,6 +411,16 @@ extern int num_passes;
 				   (HAYSTACK), (NEEDLE));		\
   SELFTEST_END_STMT
 
+/* 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.  */
+
+#define ASSERT_STR_STARTSWITH(STR, PREFIX)				    \
+  SELFTEST_BEGIN_STMT							    \
+  ::selftest::assert_str_startswith (SELFTEST_LOCATION, #STR, #PREFIX,	    \
+				     (STR), (PREFIX));			    \
+  SELFTEST_END_STMT
+
 /* Evaluate PRED1 (VAL1), calling ::selftest::pass if it is true,
    ::selftest::fail if it is false.  */

On Fri, 2018-06-22 at 13:25 +0200, Martin Liška wrote:
> On 06/20/2018 05:27 PM, David Malcolm wrote:
> > On Mon, 2018-05-14 at 14:51 +0200, Martin Liška wrote:

[...snip...]

> > Thanks for working on this; the rest looks good to me (though as I
> > said, I'm not officially a reviewer for this).

FWIW I noticed a few other minor nits in the revised version of the
patch, all just in comments:

> diff --git a/gcc/gcc.c b/gcc/gcc.c
> index dda1fd35398..9e5b153bc53 100644
> --- a/gcc/gcc.c
> +++ b/gcc/gcc.c
> @@ -221,6 +221,10 @@ static int print_help_list;
>  
>  static int print_version;
>  
> +/* Flag that stores string value for which we provide bash completion.  */
> +
> +static const char *completion = NULL;
> +

Maybe reword "value" to "prefix" in the above?


[...snip...]

> diff --git a/gcc/selftest.c b/gcc/selftest.c
> index 74adc63e65c..78ae778ca14 100644
> --- a/gcc/selftest.c
> +++ b/gcc/selftest.c
> @@ -125,6 +125,39 @@ assert_str_contains (const location &loc,
>  	 desc_haystack, desc_needle, val_haystack, val_needle);
>  }
>  
> +/* Implementation detail of ASSERT_STR_STARTSWITH.
> +   Use strstr to determine if val_haystack starts with val_needle.
> +   ::selftest::pass if it starts.
> +   ::selftest::fail if it does not start.  */
> +
> +void
> +assert_str_startswith (const location &loc,
> +		       const char *desc_str,
> +		       const char *desc_prefix,
> +		       const char *val_str,
> +		       const char *val_prefix)
> +{

The above leading comment is out of date, maybe change it to:

/* Implementation detail of ASSERT_STR_STARTSWITH.
   Determine if VAL_STR starts with VAL_PREFIX.
   ::selftest::pass if VAL_STR does start with VAL_PREFIX.
   ::selftest::fail if it does not, or either is NULL (using
   DESC_STR and DESC_PREFIX in the error message).  */


> +/* 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.  */
> +
> +#define ASSERT_STR_STARTSWITH(STR, PREFIX)				    \
> +  SELFTEST_BEGIN_STMT							    \
> +  ::selftest::assert_str_startswith (SELFTEST_LOCATION, #STR, #PREFIX,	    \
> +				     (STR), (PREFIX));			    \
> +  SELFTEST_END_STMT
> +

The argnames in the leading comment don't match those in the macro
itself; change the usages of STRING in the leading comment to STR.


Dave

On 06/22/2018 05:25 AM, Martin Liška wrote:
> On 06/20/2018 05:27 PM, David Malcolm wrote:
>> 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)
> 
> Added.
> 
>>
>> [...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)
> 
> Fixed that, should not be removed.
> 
>>  
>> +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?
> 
> Thanks, done.
> 
>>
>> [...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.
> 
> Likewise.
> 
>>
>> [...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.
> 
> Makes sense.
> 
>>
>> Missing leading comment.  Maybe something like:
>>
>> /* Populate RESULTS with bash-completions options for all parameters
>>    that begin with PARAM_PREFIX, using SEPARATOR.
> 
> It's in header file, thus I copied that.
> 
>>
>>    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?)
> 
> Yes.
> 
>>
>> +{
>> +  char separator_str[] {separator, '\0'};
>>
>> Is the above initialization valid C++98, or is it a C++11-ism?
> 
> You are right. I fixed that and 2 more occurrences of the same
> issue.
> 
>>
>> +  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.  */
> 
> Yes!
> 
>>
>> +  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?
> 
> Fixed.
> 
>>
>>  
>> +  /* 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.
> 
> Done.
> 
>>
>>  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).
> 
> Thanks for the review.
> 
>>
>> Hope this is constructive
> 
> Yes, very.
So if David is OK with this version, so am I.

jeff

On 06/22/2018 06:09 PM, Jeff Law wrote:
> On 06/22/2018 05:25 AM, Martin Liška wrote:
>> On 06/20/2018 05:27 PM, David Malcolm wrote:
>>> 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)
>>
>> Added.
>>
>>>
>>> [...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)
>>
>> Fixed that, should not be removed.
>>
>>>  
>>> +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?
>>
>> Thanks, done.
>>
>>>
>>> [...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.
>>
>> Likewise.
>>
>>>
>>> [...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.
>>
>> Makes sense.
>>
>>>
>>> Missing leading comment.  Maybe something like:
>>>
>>> /* Populate RESULTS with bash-completions options for all parameters
>>>    that begin with PARAM_PREFIX, using SEPARATOR.
>>
>> It's in header file, thus I copied that.
>>
>>>
>>>    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?)
>>
>> Yes.
>>
>>>
>>> +{
>>> +  char separator_str[] {separator, '\0'};
>>>
>>> Is the above initialization valid C++98, or is it a C++11-ism?
>>
>> You are right. I fixed that and 2 more occurrences of the same
>> issue.
>>
>>>
>>> +  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.  */
>>
>> Yes!
>>
>>>
>>> +  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?
>>
>> Fixed.
>>
>>>
>>>  
>>> +  /* 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.
>>
>> Done.
>>
>>>
>>>  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).
>>
>> Thanks for the review.
>>
>>>
>>> Hope this is constructive
>>
>> Yes, very.
> So if David is OK with this version, so am I.
> 
> jeff
> 

Thanks for the review. I hope I included all David's notes and I'm
going to install the patch. I'm planning to incrementally add DejaGNU
tests for the functionality.

Martin

Hi.

I would like to link bash-completion pull request that adjusts gcc option provides:
https://github.com/scop/bash-completion/pull/222

Martin