This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: RFC: bash completion
- From: David Malcolm <dmalcolm at redhat dot com>
- To: Martin Liška <mliska at suse dot cz>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 25 Apr 2018 19:13:31 -0400
- Subject: Re: RFC: bash completion
- References: <3c2e7f32-438a-57a3-96a8-3e9d3fc7f605@suse.cz> <1524587261.5688.193.camel@redhat.com> <cabd9b8d-d5a4-b4e4-a28a-07f020a3288a@suse.cz>
[moving from gcc to gcc-patches mailing list]
On Wed, 2018-04-25 at 15:12 +0200, Martin Liška wrote:
> On 04/24/2018 06:27 PM, David Malcolm wrote:
> > On Tue, 2018-04-24 at 16:45 +0200, Martin Liška wrote:
> > > Hi.
> > >
> > > Some time ago, I investigated quite new feature of clang which
> > > is support of --autocomplete argument. That can be run from bash
> > > completion
> > > script and one gets more precise completion hints:
> > >
> > > http://blog.llvm.org/2017/09/clang-bash-better-auto-completion-is
> > > .htm
> > > l
> > > https://www.youtube.com/watch?v=zLPwPdZBpSY
> > >
> > > I like the idea and I would describe how is that better to
> > > current
> > > GCC completion
> > > script sitting here:
> > > https://github.com/scop/bash-completion/blob/master/completions/g
> > > cc
> > >
> > > 1) gcc -fsanitize=^ - no support for option enum values
> > > 2) gcc -fno-sa^ - no support for negative options
> > > 3) gcc --param=^ - no support for param names
> > >
> > > These are main limitations I see. I'm attaching working
> > > prototype,
> > > which you
> > > can test by installed GCC, setting it on $PATH and doing:
> > > $ source gcc.sh
> > >
> > > Then bash completion is provided via the newly added option. Some
> > > examples:
> > >
> > > 1)
> > > $ gcc -fsanitize=
> > > address bounds enum
> > >
> > > integer-divide-by-zero nonnull-
> > > attribute pointer-
> > > compare return shift-
> > > base thread vla-bound
> > > alignment bounds-strict float-cast-
> > > overflow kernel-
> > > address null pointer-
> > > overflow returns-nonnull-attribute shift-
> > > exponent undefined vptr
> > > bool builtin float-
> > > divide-
> > > by-zero leak object-
> > > size pointer-
> > > subtract shift signed-integer-
> > > overflow unreachable
> > >
> > > 2)
> > > $ gcc -fno-ipa-
> > > -fno-ipa-bit-cp -fno-ipa-cp-alignment -fno-ipa-
> > > icf -fno-ipa-icf-variables -fno-ipa-profile -
> > > fno-
> > > ipa-pure-const -fno-ipa-reference -fno-ipa-struct-
> > > reorg
> > > -fno-ipa-cp -fno-ipa-cp-clone -fno-ipa-icf-
> > > functions -fno-ipa-matrix-reorg -fno-ipa-pta -fno-
> > > ipa-
> > > ra -fno-ipa-sra -fno-ipa-vrp
> > >
> > > 3)
> > > $ gcc --param=lto-
> > > lto-max-partition lto-min-partition lto-partitions
> > >
> > > 4)
> > > gcc --param lto-
> > > lto-max-partition lto-min-partition lto-partitions
> > >
> > > The patch is quite lean and if people like, I will prepare a
> > > proper
> > > patch submission. I know about some limitations that can be then
> > > handled incrementally.
> > >
> > > Thoughts?
> > > Martin
> >
> > Overall, looks good (albeit with various nits). I like how you're
> > reusing the m_option_suggestions machinery from the misspelled
> > options
> > code. There are some awkward issues e.g. arch-specific
> > completions,
> > lang-specific completions, custom option-handling etc, but given
> > that
> > as-is this patch seems to be an improvement over the status quo,
> > I'd
> > prefer to tackle those later.
>
> I'm sending second version of the patch. I did isolation of
> m_option_suggestions machinery
> to a separate file. Mainly due to selftests that are linked with cc1.
>
> >
> > The patch doesn't have tests. There would need to be some way to
> > achieve test coverage for the completion code (especially as we
> > start
> > to tackle the more interesting cases). I wonder what the best way
> > to
> > do that is; perhaps a combination of selftest and DejaGnu? (e.g.
> > what
> > about arch-specific completions? what about the interaction with
> > bash?
> > etc)
>
> For now I come up with quite some selftests. Integration with
> bash&DejaGNU
> would be challenging.
>
> >
> > A few nits:
> > * Do we want to hardcode that logging path in gcc.sh?
>
> Sure, that needs to be purged. Crucial question here is where the
> gcc.sh script
> should live. Note that clang has it in: ./tools/clang/utils/bash-
> autocomplete.sh
> and:
>
> head -n1 ./tools/clang/utils/bash-autocomplete.sh
> # Please add "source /path/to/bash-autocomplete.sh" to your .bashrc
> to use this.
>
> Which is not ideal. I would prefer to integrate the script into:
> https://github.com/scop/bash-completion/blob/master/completions/gcc
>
> Thoughts?
Maybe our goal should be to update that upstream bash-completion script
so that it uses "--completion" if it exists (for newer GCCs), falling
back to their existing implementation if it doesn't?
> >
> > * Looks like m_option_suggestions isn't needed for handling the "-
> > param" case, so maybe put the param-handling case before the
> > "Lazily
> > populate m_option_suggestions" code.
> >
> > * You use "l" ("ell") as a variable name in two places, which I
> > don't
> > like, as IMHO it's too close to "1" (one) in some fonts.
>
> Fixed both notes.
> Thanks for fast review.
Here's a review of/suggested improvements for the updated patch
(technically I'm not a reviewer for this code, although I am the author
for the existing options-spellchecking code).
Missing ChangeLog, though I get that it was a proof-of-concept.
Is it possible to split the moves of the existing code out from the
rest of the patch as a preliminary patch?
> diff --git a/gcc.sh b/gcc.sh
> new file mode 100644
> index 00000000000..06b16b3152b
> --- /dev/null
> +++ b/gcc.sh
(I won't attempt to review the bash script for now; I'm not a bash expert)
> diff --git a/gcc/Makefile.in b/gcc/Makefile.in
> index 20bee0494b1..26fa3dd17df 100644
> --- a/gcc/Makefile.in
> +++ b/gcc/Makefile.in
> @@ -1617,7 +1617,7 @@ OBJS-libcommon = diagnostic.o diagnostic-color.o diagnostic-show-locus.o \
> # compiler and containing target-dependent code.
> OBJS-libcommon-target = $(common_out_object_file) prefix.o params.o \
> opts.o opts-common.o options.o vec.o hooks.o common/common-targhooks.o \
> - hash-table.o file-find.o spellcheck.o selftest.o
> + hash-table.o file-find.o spellcheck.o selftest.o opt-proposer.o
I don't love the "opt-proposer" name; maybe "opt-suggestions.o"? (not sure)
> # This lists all host objects for the front ends.
> ALL_HOST_FRONTEND_OBJS = $(foreach v,$(CONFIG_LANGUAGES),$($(v)_OBJS))
> diff --git a/gcc/c-family/cppspec.c b/gcc/c-family/cppspec.c
> index 1e0a8bcd294..794b3ced529 100644
> --- a/gcc/c-family/cppspec.c
> +++ b/gcc/c-family/cppspec.c
> @@ -21,6 +21,7 @@ along with GCC; see the file COPYING3. If not see
> #include "system.h"
> #include "coretypes.h"
> #include "tm.h"
> +#include "opt-proposer.h"
> #include "gcc.h"
> #include "opts.h"
I think I'd prefer "opt-suggestions.h" here. Not sure.
I don't understand our policies for how me manage #include files - if
gcc.h needs a header to compile, isn't it simpler to add that to gcc.h
itself? (encoding that dependency in one place, rather than in
everything that uses gcc.h)
>
> diff --git a/gcc/common.opt b/gcc/common.opt
> index d6ef85928f3..9b4ba28f287 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -254,6 +254,10 @@ Driver Alias(S)
> -compile
> Driver Alias(c)
>
> +-completion=
> +Common Driver Joined Undocumented
> +--param Bash completion.
> +
Is this description correct? This is for much more than just "--param", isn't it?
> -coverage
> Driver Alias(coverage)
>
> diff --git a/gcc/fortran/gfortranspec.c b/gcc/fortran/gfortranspec.c
> index fe1ec0447b3..b95c8810d0f 100644
> --- a/gcc/fortran/gfortranspec.c
> +++ b/gcc/fortran/gfortranspec.c
> @@ -47,6 +47,7 @@ along with GCC; see the file COPYING3. If not see
> #include "config.h"
> #include "system.h"
> #include "coretypes.h"
> +#include "opt-proposer.h"
> #include "gcc.h"
> #include "opts.h"
>
> diff --git a/gcc/gcc-main.c b/gcc/gcc-main.c
> index 9e6de743adc..3cfdfdc57fa 100644
> --- a/gcc/gcc-main.c
> +++ b/gcc/gcc-main.c
> @@ -30,6 +30,7 @@ along with GCC; see the file COPYING3. If not see
> #include "obstack.h"
> #include "intl.h"
> #include "prefix.h"
> +#include "opt-proposer.h"
> #include "gcc.h"
>
> /* Implement the top-level "main" within the driver in terms of
> diff --git a/gcc/gcc.c b/gcc/gcc.c
> index a716f708259..e9207bb9823 100644
> --- a/gcc/gcc.c
> +++ b/gcc/gcc.c
> @@ -36,6 +36,7 @@ compilation is specified by a string called a "spec". */
> #include "obstack.h"
> #include "intl.h"
> #include "prefix.h"
> +#include "opt-proposer.h"
> #include "gcc.h"
> #include "diagnostic.h"
> #include "flags.h"
> @@ -220,6 +221,8 @@ static int print_help_list;
>
> static int print_version;
>
> +static const char *completion = NULL;
> +
Please give this variable a descriptive comment.
> /* 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
> @@ -3818,6 +3821,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;
>
> @@ -7262,8 +7270,7 @@ compare_files (char *cmpfile[])
>
> driver::driver (bool can_finalize, bool debug) :
> explicit_link_files (NULL),
> - decoded_options (NULL),
> - m_option_suggestions (NULL)
> + decoded_options (NULL)
> {
> env.init (can_finalize, debug);
> }
> @@ -7272,14 +7279,6 @@ driver::~driver ()
> {
> XDELETEVEC (explicit_link_files);
> XDELETEVEC (decoded_options);
> - if (m_option_suggestions)
> - {
> - int i;
> - char *str;
> - FOR_EACH_VEC_ELT (*m_option_suggestions, i, str)
> - free (str);
> - delete m_option_suggestions;
> - }
> }
>
> /* driver::main is implemented as a series of driver:: method calls. */
> @@ -7300,6 +7299,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;
>
> @@ -7768,106 +7773,6 @@ driver::maybe_putenv_OFFLOAD_TARGETS () const
> offload_targets = NULL;
> }
>
> -/* Helper function for driver::suggest_option. Populate
> - m_option_suggestions with candidate strings for misspelled options.
> - The strings will be freed by the driver's dtor. */
> -
> -void
> -driver::build_option_suggestions (void)
> -{
> - gcc_assert (m_option_suggestions == NULL);
> - m_option_suggestions = new auto_vec <char *> ();
> -
> - /* We build a vec of m_option_suggestions, using add_misspelling_candidates
> - to add copies of strings, without a leading dash. */
> -
> - for (unsigned int i = 0; i < cl_options_count; i++)
> - {
> - const struct cl_option *option = &cl_options[i];
> - const char *opt_text = option->opt_text;
> - switch (i)
> - {
> - default:
> - if (option->var_type == CLVC_ENUM)
> - {
> - const struct cl_enum *e = &cl_enums[option->var_enum];
> - for (unsigned j = 0; e->values[j].arg != NULL; j++)
> - {
> - char *with_arg = concat (opt_text, e->values[j].arg, NULL);
> - add_misspelling_candidates (m_option_suggestions, option,
> - with_arg);
> - free (with_arg);
> - }
> - }
> - else
> - add_misspelling_candidates (m_option_suggestions, option,
> - opt_text);
> - break;
> -
> - case OPT_fsanitize_:
> - case OPT_fsanitize_recover_:
> - /* -fsanitize= and -fsanitize-recover= can take
> - a comma-separated list of arguments. Given that combinations
> - are supported, we can't add all potential candidates to the
> - vec, but if we at least add them individually without commas,
> - we should do a better job e.g. correcting
> - "-sanitize=address"
> - to
> - "-fsanitize=address"
> - rather than to "-Wframe-address" (PR driver/69265). */
> - {
> - for (int j = 0; sanitizer_opts[j].name != NULL; ++j)
> - {
> - struct cl_option optb;
> - /* -fsanitize=all is not valid, only -fno-sanitize=all.
> - So don't register the positive misspelling candidates
> - for it. */
> - if (sanitizer_opts[j].flag == ~0U && i == OPT_fsanitize_)
> - {
> - optb = *option;
> - optb.opt_text = opt_text = "-fno-sanitize=";
> - optb.cl_reject_negative = true;
> - option = &optb;
> - }
> - /* Get one arg at a time e.g. "-fsanitize=address". */
> - char *with_arg = concat (opt_text,
> - sanitizer_opts[j].name,
> - NULL);
> - /* Add with_arg and all of its variant spellings e.g.
> - "-fno-sanitize=address" to candidates (albeit without
> - leading dashes). */
> - add_misspelling_candidates (m_option_suggestions, option,
> - with_arg);
> - free (with_arg);
> - }
> - }
> - break;
> - }
> - }
> -}
> -
> -/* Helper function for driver::handle_unrecognized_options.
> -
> - Given an unrecognized option BAD_OPT (without the leading dash),
> - locate the closest reasonable matching option (again, without the
> - leading dash), or NULL.
> -
> - The returned string is owned by the driver instance. */
> -
> -const char *
> -driver::suggest_option (const char *bad_opt)
> -{
> - /* Lazily populate m_option_suggestions. */
> - if (!m_option_suggestions)
> - build_option_suggestions ();
> - gcc_assert (m_option_suggestions);
> -
> - /* "m_option_suggestions" is now populated. Use it. */
> - return find_closest_string
> - (bad_opt,
> - (auto_vec <const char *> *) m_option_suggestions);
> -}
> -
> /* Reject switches that no pass was interested in. */
>
> void
> @@ -7876,7 +7781,7 @@ driver::handle_unrecognized_options ()
> for (size_t i = 0; (int) i < n_switches; i++)
> if (! switches[i].validated)
> {
> - const char *hint = suggest_option (switches[i].part1);
> + const char *hint = m_option_proposer.suggest_option (switches[i].part1);
> if (hint)
> error ("unrecognized command line option %<-%s%>;"
> " did you mean %<-%s%>?",
> diff --git a/gcc/gcc.h b/gcc/gcc.h
> index ddbf42f78ea..a7606183393 100644
> --- a/gcc/gcc.h
> +++ b/gcc/gcc.h
> @@ -45,8 +45,6 @@ class driver
> void putenv_COLLECT_GCC (const char *argv0) const;
> void maybe_putenv_COLLECT_LTO_WRAPPER () const;
> void maybe_putenv_OFFLOAD_TARGETS () const;
> - void build_option_suggestions (void);
> - const char *suggest_option (const char *bad_opt);
> void handle_unrecognized_options ();
> int maybe_print_and_exit () const;
> bool prepare_infiles ();
> @@ -59,7 +57,7 @@ class driver
> char *explicit_link_files;
> struct cl_decoded_option *decoded_options;
> unsigned int decoded_options_count;
> - auto_vec <char *> *m_option_suggestions;
> + option_proposer m_option_proposer;
FWIW I'd prefer:
option_suggestions m_option_suggestions;
but proposer is a better fit, I guess.
> };
>
> /* The mapping of a spec function name to the C function that
> diff --git a/gcc/opt-proposer.c b/gcc/opt-proposer.c
> new file mode 100644
> index 00000000000..08379f0b631
> --- /dev/null
> +++ b/gcc/opt-proposer.c
> @@ -0,0 +1,420 @@
> +/* Provide option suggestion for --complete option and a misspelled
> + used by a user.
Maybe reword to:
/* Provide suggestions to handle misspelled options, and implement the
--complete option for auto-completing options from a prefix. */
or somesuch.
> + Copyright (C) 2018 Free Software Foundation, Inc.
This new file contains older material that was in gcc.c; I believe the
earliest such material that you're copying and pasting is from r233382,
which was from 2016, so the copyright date should be the range 2016-2018,
if I understand our policies here correctly.
> +This file is part of GCC.
> +
> +GCC is free software; you can redistribute it and/or modify it under
> +the terms of the GNU General Public License as published by the Free
> +Software Foundation; either version 3, or (at your option) any later
> +version.
> +
> +GCC is distributed in the hope that it will be useful, but WITHOUT ANY
> +WARRANTY; without even the implied warranty of MERCHANTABILITY or
> +FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> +for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with GCC; see the file COPYING3. If not see
> +<http://www.gnu.org/licenses/>. */
> +
> +#include "config.h"
> +#include "system.h"
> +#include "coretypes.h"
> +#include "tm.h"
> +#include "opts.h"
> +#include "params.h"
> +#include "spellcheck.h"
> +#include "opt-proposer.h"
> +#include "selftest.h"
> +
> +option_proposer::~option_proposer ()
Needs a leading descriptive comment; maybe just use:
/* option_proposer's dtor. */
> +{
> + if (m_option_suggestions)
> + {
> + int i;
> + char *str;
> + FOR_EACH_VEC_ELT (*m_option_suggestions, i, str)
> + free (str);
> + delete m_option_suggestions;
> + }
> +
> + release_completion_results ();
Presumably you're stashing the results to avoid having to deal with lifetime
issues of the strings when running the selftests?
Or is it due to the misspellings code expecting an implicit leading "-",
whereas the completion code doesn't have an implicit leading "--"?
It seems like it would be simpler to have a class that's a managed vec of
strings: we already nearly have one for m_option_suggestion? (auto_string_vec
or somesuch? See class auto_argvec in jit/jit-playback.c).
If so, then the option_proposer's data would simply be two instance of
this managed vec; the cleanup logic would be in auto_string_vec's dtor.
Alternatively, it seems a bit weird to have the option_proposer own the
results. Maybe have the caller pass in an auto_string_vec& to be
populated, and thus have the caller manage the lifetime of the results:
void
option_proposer::get_completions (const char *option_prefix,
auto_string_vec &out)
(Better would be to return an auto_string_vec, but given that we're
stuck on C++98 it seems safest to pass it in by reference as an out-var).
> +}
>
> +const char *
> +option_proposer::suggest_option (const char *bad_opt)
> +{
Needs a leading descriptive comment; maybe use:
/* Given an unrecognized option BAD_OPT (without the leading dash),
locate the closest reasonable matching option (again, without the
leading dash), or NULL.
The returned string is owned by the option_proposer instance. */
(I adapted this from the older code; is it still true?)
> + /* Lazily populate m_option_suggestions. */
> + if (!m_option_suggestions)
> + build_option_suggestions ();
> + gcc_assert (m_option_suggestions);
> +
> + /* "m_option_suggestions" is now populated. Use it. */
> + return find_closest_string
> + (bad_opt,
> + (auto_vec <const char *> *) m_option_suggestions);
> +}
> +void
> +option_proposer::build_completions (const char *starting)
> +{
Needs a leading descriptive comment.
Maybe "option_prefix" rather than "starting"?
> + release_completion_results ();
> +
> + /* Bail out for an invalid input. */
> + if (starting == NULL || starting[0] == '\0')
> + return;
> +
> + if (starting[0] == '-')
> + starting++;
What's the purpose of the above logic? (a genuine question)
Maybe it warrants a comment?
> +
> + size_t length = strlen (starting);
> +
> + /* Handle parameters. */
> + const char *prefix = "-param";
> + if (length >= strlen (prefix) && strstr (starting, prefix) == starting)
> + {
> + /* We support both '-param-xyz=123' and '-param xyz=123' */
> + starting += strlen (prefix);
> + char separator = starting[0];
> + starting++;
> + if (separator == ' ' || separator == '=')
> + find_param_completions (separator, starting);
> + }
> + 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, starting) == candidate)
> + m_completion_results.safe_push (concat ("-", candidate, NULL));
> + }
> + }
> +}
> +
> +void
> +option_proposer::suggest_completion (const char *starting)
Needs a leading descriptive comment.
> +{
> + build_completions (starting);
> + print_completion_results ();
> + release_completion_results ();
> +}
> +
> +auto_vec <char *> *
> +option_proposer::get_completion_suggestions (const char *starting)
Needs a leading descriptive comment. Presumably this purely exists for
the selftests, right? Can it be a "const"?
> +{
> + build_completions (starting);
> + return &m_completion_results;
> +}
> +
> +void
> +option_proposer::build_option_suggestions (void)
Needs a leading descriptive comment; possibly something like:
/* Populate m_option_suggestions with candidate strings for misspelled options.
The strings will be freed by the driver's dtor. */
or somesuch (adapted from the older code).
> +{
> + gcc_assert (m_option_suggestions == NULL);
> + m_option_suggestions = new auto_vec <char *> ();
> +
> + /* We build a vec of m_option_suggestions, using add_misspelling_candidates
> + to add copies of strings, without a leading dash. */
> +
> + for (unsigned int i = 0; i < cl_options_count; i++)
> + {
> + const struct cl_option *option = &cl_options[i];
> + const char *opt_text = option->opt_text;
> + switch (i)
> + {
> + default:
> + if (option->var_type == CLVC_ENUM)
> + {
> + const struct cl_enum *e = &cl_enums[option->var_enum];
> + for (unsigned j = 0; e->values[j].arg != NULL; j++)
> + {
> + char *with_arg = concat (opt_text, e->values[j].arg, NULL);
> + add_misspelling_candidates (m_option_suggestions, option,
> + with_arg);
> + free (with_arg);
> + }
> + }
> + else
> + add_misspelling_candidates (m_option_suggestions, option,
> + opt_text);
> + break;
> +
> + case OPT_fsanitize_:
> + case OPT_fsanitize_recover_:
> + /* -fsanitize= and -fsanitize-recover= can take
> + a comma-separated list of arguments. Given that combinations
> + are supported, we can't add all potential candidates to the
> + vec, but if we at least add them individually without commas,
> + we should do a better job e.g. correcting
> + "-sanitize=address"
> + to
> + "-fsanitize=address"
> + rather than to "-Wframe-address" (PR driver/69265). */
> + {
> + for (int j = 0; sanitizer_opts[j].name != NULL; ++j)
> + {
> + struct cl_option optb;
> + /* -fsanitize=all is not valid, only -fno-sanitize=all.
> + So don't register the positive misspelling candidates
> + for it. */
> + if (sanitizer_opts[j].flag == ~0U && i == OPT_fsanitize_)
> + {
> + optb = *option;
> + optb.opt_text = opt_text = "-fno-sanitize=";
> + optb.cl_reject_negative = true;
> + option = &optb;
> + }
> + /* Get one arg at a time e.g. "-fsanitize=address". */
> + char *with_arg = concat (opt_text,
> + sanitizer_opts[j].name,
> + NULL);
> + /* Add with_arg and all of its variant spellings e.g.
> + "-fno-sanitize=address" to candidates (albeit without
> + leading dashes). */
> + add_misspelling_candidates (m_option_suggestions, option,
> + with_arg);
> + free (with_arg);
> + }
> + }
> + break;
> + }
> + }
> +}
> +
> +void
> +option_proposer::find_param_completions (const char separator,
> + const char *starting)
> +{
Needs a leading descriptive comment.
> + char separator_str[] {separator, '\0'};
> + size_t length = strlen (starting);
> + for (unsigned i = 0; i < get_num_compiler_params (); ++i)
> + {
> + const char *candidate = compiler_params[i].option;
> + if (strlen (candidate) >= length
> + && strstr (candidate, starting) == candidate)
> + m_completion_results.safe_push (concat ("--param", separator_str,
> + candidate, NULL));
> + }
> +}
> +
> +void
> +option_proposer::print_completion_results ()
> +{
Needs a leading descriptive comment.
> + for (unsigned i = 0; i < m_completion_results.length (); i++)
> + printf ("%s\n", m_completion_results[i]);
> +}
> +
> +void
> +option_proposer::release_completion_results ()
> +{
Needs a leading descriptive comment.
> + for (unsigned i = 0; i < m_completion_results.length (); i++)
> + free (m_completion_results[i]);
> +
> + m_completion_results.truncate (0);
> +}
> +
> +#if CHECKING_P
> +
> +namespace selftest {
> +
> +static void
> +test_valid_option (option_proposer &proposer, const char *option)
Needs a leading descriptive comment. Maybe rename option to "starting",
or to "option_prefix".
/* Verify that PROPOSER generate sane auto-completion suggestions for STARTING. */
Maybe rename the function e.g. to verify_autocompletions or somesuch.
> +{
> + auto_vec <char *> *suggestions = proposer.get_completion_suggestions (option);
> + ASSERT_GT (suggestions->length (), 0);
> +
> + for (unsigned i = 0; i < suggestions->length (); i++)
> + ASSERT_STR_STARTSWITH ((*suggestions)[i], option);
> +}
> +
> +/* Verify that valid options works correctly. */
How about:
/* Verify that valid options are auto-completed correctly. */
> +
> +static void
> +test_completion_valid_options (option_proposer &proposer)
> +{
> + const char *needles[]
I like the use of "needle" and "haystack" as param names for arbitrary
string searches, but I don't like them for the prefix-based search
we're doing here.
How about "starting_strs" here, or "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 = needles; *ptr != NULL; ptr++)
> + test_valid_option (proposer, *ptr);
> +}
> +
> +/* Verify that valid parameter works correctly. */
Maybe:
/* 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 *needles[]
"starting_strs" or "option_prefixes" rather than "needles".
> + {
> + "--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 = needles; *ptr != NULL; ptr++)
> + test_valid_option (proposer, *ptr);
> +}
> +
> +/* Return true when EXPECTED is one of completions for OPTION string. */
Maybe rename "option" to "option_prefix"?
> +
> +static bool
> +in_completion_p (option_proposer &proposer, const char *option,
> + const char *expected)
> +{
> + auto_vec <char *> *suggestions = proposer.get_completion_suggestions (option);
> +
> + for (unsigned i = 0; i < suggestions->length (); i++)
> + {
> + char *r = (*suggestions)[i];
> + if (strcmp (r, expected) == 0)
> + return true;
> + }
> +
> + return false;
> +}
Going with the above ideas on ownership, passing in an auto_string_vec &,
this might look like:
static bool
in_completion_p (option_proposer &proposer, const char *option_prefix,
const char *expected)
{
auto_string_vec suggestions;
proposer.get_completion_suggestions (suggestions, option_prefix);
for (unsigned i = 0; i < suggestions->length (); i++)
{
char *r = (*suggestions)[i];
if (strcmp (r, expected) == 0)
return true;
}
return false;
}
> +static bool
> +empty_completion_p (option_proposer &proposer, const char *option)
> +{
Needs leading comment; maybe rename to "option_prefix".
> + auto_vec <char *> *suggestions = proposer.get_completion_suggestions (option);
> + return suggestions->is_empty ();
> +}
With ownership ideas above, might look like:
static bool
empty_completion_p (option_proposer &proposer, const char *option_prefix)
{
auto_string_vec suggestions;
proposer.get_completion_suggestions (suggestions, option_prefix);
return suggestions->is_empty ();
}
> +/* Verify partial completions. */
Maybe:
/* 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"));
> +}
> +
> +/* Verify that a garbage does not return a completion match. */
Maybe:
/* 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"));
> +
> + ASSERT_FALSE (empty_completion_p (proposer, "-"));
> + ASSERT_FALSE (empty_completion_p (proposer, "-fipa"));
> + ASSERT_FALSE (empty_completion_p (proposer, "--par"));
Maybe move these ASSERT_FALSE cases to test_completion_partial_match?
> +}
> +
> +/* 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-proposer.h b/gcc/opt-proposer.h
> new file mode 100644
> index 00000000000..b673f02dc70
> --- /dev/null
> +++ b/gcc/opt-proposer.h
> @@ -0,0 +1,84 @@
> +/* Provide option suggestion for --complete option and a misspelled
> + used by a user.
Maybe reword as per opt-proposer.c above.
> + Copyright (C) 2018 Free Software Foundation, Inc.
2016-2018, I think.
> +
> +This file is part of GCC.
> +
> +GCC is free software; you can redistribute it and/or modify it under
> +the terms of the GNU General Public License as published by the Free
> +Software Foundation; either version 3, or (at your option) any later
> +version.
> +
> +GCC is distributed in the hope that it will be useful, but WITHOUT ANY
> +WARRANTY; without even the implied warranty of MERCHANTABILITY or
> +FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> +for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with GCC; see the file COPYING3. If not see
> +<http://www.gnu.org/licenses/>. */
> +
> +#ifndef GCC_OPT_PROPOSER_H
> +#define GCC_OPT_PROPOSER_H
> +
> +/* Option proposer is class used by driver in order to provide hints
> + for wrong options provided. And it's used by --complete option that's
> + intended to be invoked by BASH in order to provide better option
> + completion support. */
> +
> +class option_proposer
> +{
> +public:
> + /* Default constructor. */
> + option_proposer (): m_option_suggestions (NULL), m_completion_results ()
> + {}
> +
> + /* Default destructor. */
> + ~option_proposer ();
> +
> + /* Helper function for driver::handle_unrecognized_options.
> +
> + Given an unrecognized option BAD_OPT (without the leading dash),
> + locate the closest reasonable matching option (again, without the
> + leading dash), or NULL.
> +
> + The returned string is owned by the option_proposer instance. */
> + const char *suggest_option (const char *bad_opt);
> +
> + /* Print to stdout all options that start with STARTING. */
> + void suggest_completion (const char *starting);
> +
> + /* Return vector with completion suggestions that start with STARTING.
> +
> + The returned strings are owned by the option_proposer instance. */
> + auto_vec <char *> *get_completion_suggestions (const char *starting);
> +
> +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 ();
> +
> + /* Build completions that start with STARTING and save them
> + into m_completion_results vector. */
> + void build_completions (const char *starting);
> +
> + /* Find parameter completions for --param format with SEPARATOR.
> + Again, save the completions into m_completion_results. */
> + void find_param_completions (const char separator, const char *starting);
> +
> + /* Print found completions in m_completion_results to stdout. */
> + void print_completion_results ();
> +
> + /* Free content of m_completion_results. */
> + void release_completion_results ();
> +
> +private:
> + /* Cache with all suggestions. */
> + auto_vec <char *> *m_option_suggestions;
> +
> + /* Completion cache. */
> + auto_vec <char *> m_completion_results;
> +};
> +
> +#endif /* GCC_OPT_PROPOSER_H */
> 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 5709110c291..4cff89d2909 100644
> --- a/gcc/selftest.c
> +++ b/gcc/selftest.c
> @@ -118,6 +118,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_haystack,
> + const char *desc_needle,
> + const char *val_haystack,
> + const char *val_needle)
I don't think we should use "haystack" and "needle" for prefix-based
string searches.
Maybe rename "haystack" to "str" and "needle" to "prefix".
> +{
> + /* If val_haystack is NULL, fail with a custom error message. */
> + if (val_haystack == NULL)
> + fail_formatted (loc, "ASSERT_STR_STARTSWITH (%s, %s) haystack=NULL",
> + desc_haystack, desc_needle);
> +
> + /* If val_needle is NULL, fail with a custom error message. */
> + if (val_needle == NULL)
> + fail_formatted (loc,
> + "ASSERT_STR_STARTSWITH (%s, %s) haystack=\"%s\" needle=NULL",
> + desc_haystack, desc_needle, val_haystack);
> +
> + const char *test = strstr (val_haystack, val_needle);
> + if (test == val_haystack)
> + pass (loc, "ASSERT_STR_STARTSWITH");
> + else
> + fail_formatted
> + (loc, "ASSERT_STR_STARTSWITH (%s, %s) haystack=\"%s\" needle=\"%s\"",
> + desc_haystack, desc_needle, val_haystack, val_needle);
> +}
> +
> +
> /* 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 e3117c6bfc4..b5d7eeef86e 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_expected,
> + const char *desc_actual,
> + const char *val_expected,
> + const char *val_actual);
Rename params for consistency with the above.
> +
> /* 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 wide_int_cc_tests ();
> extern void predict_c_tests ();
> extern void simplify_rtx_c_tests ();
> extern void vec_perm_indices_c_tests ();
> +extern void opt_proposer_c_tests ();
>
> extern int num_passes;
>
> @@ -401,6 +411,17 @@ extern int num_passes;
> (HAYSTACK), (NEEDLE)); \
> SELFTEST_END_STMT
>
> +/* Evaluate HAYSTACK and NEEDLE and use strstr to determine if HAYSTACK
> + starts with NEEDLE.
> + ::selftest::pass if starts.
> + ::selftest::fail if does not start. */
> +
> +#define ASSERT_STR_STARTSWITH(HAYSTACK, NEEDLE) \
> + SELFTEST_BEGIN_STMT \
> + ::selftest::assert_str_startswith (SELFTEST_LOCATION, #HAYSTACK, #NEEDLE, \
> + (HAYSTACK), (NEEDLE)); \
> + SELFTEST_END_STMT
Likewise.
> /* Evaluate PRED1 (VAL1), calling ::selftest::pass if it is true,
> ::selftest::fail if it is false. */
Hope this is constructive
Dave