* Re: RFC: bash completion
[not found] ` <cabd9b8d-d5a4-b4e4-a28a-07f020a3288a@suse.cz>
@ 2018-04-26 1:28 ` David Malcolm
2018-05-14 12:48 ` Martin Liška
0 siblings, 1 reply; 19+ messages in thread
From: David Malcolm @ 2018-04-26 1:28 UTC (permalink / raw)
To: Martin Liška, gcc-patches
[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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: RFC: bash completion
2018-04-26 1:28 ` RFC: bash completion David Malcolm
@ 2018-05-14 12:48 ` Martin Liška
2018-05-14 12:51 ` [PATCH 1/3] Introduce auto_string_vec class Martin Liška
` (4 more replies)
0 siblings, 5 replies; 19+ messages in thread
From: Martin Liška @ 2018-05-14 12:48 UTC (permalink / raw)
To: David Malcolm, gcc-patches
On 04/26/2018 01:13 AM, David Malcolm wrote:
> [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?
Hi David.
Thanks a lot for so detailed review feedback. If not said otherwise I adapted
all your suggestions.
>
> 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?
Yes, I will work on that as soon as GCC's part is ready.
>
>>>
>>> * 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.
Will do that once we'll make a conclusion about the patches.
>
> 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)
Done.
>
>> # 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)
Now it's very bad, due to flat header files one needs to do the explicit inclusion
of prerequisites for a header file. Don't ask me why..
>
>>
>> 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.
There are multiple situations where I have comment in header file (opt-suggestions.h).
Is it needed to copy it to implementation? I consider it a duplicate information.
I'll send 3 patches that are split from the original one afterwards.
Thanks a lot,
Martin
>
>> + 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
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/3] Introduce auto_string_vec class.
2018-05-14 12:48 ` Martin Liška
@ 2018-05-14 12:51 ` Martin Liška
2018-06-20 14:41 ` David Malcolm
2018-05-14 12:51 ` [PATCH 2/3] Refactoring to opt-suggestions.[ch] Martin Liška
` (3 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Martin Liška @ 2018-05-14 12:51 UTC (permalink / raw)
To: David Malcolm, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 63 bytes --]
First part with introduction of auto_string_vec class.
Martin
[-- Attachment #2: 0001-Introduce-auto_string_vec-class.patch --]
[-- Type: text/x-patch, Size: 3684 bytes --]
From c05ad6d3715fcc99e94dbe2fd3fa1ef94552bea4 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Mon, 14 May 2018 14:00:07 +0200
Subject: [PATCH 1/3] Introduce auto_string_vec class.
gcc/ChangeLog:
2018-05-14 Martin Liska <mliska@suse.cz>
* vec.h (class auto_string_vec): New (moved from auto_argvec).
(auto_string_vec::~auto_string_vec): Likewise.
gcc/jit/ChangeLog:
2018-05-14 Martin Liska <mliska@suse.cz>
* jit-playback.c (class auto_argvec): Moved to vec.h.
(auto_argvec::~auto_argvec): Likewise.
(compile): Use the renamed name.
(invoke_driver): Likewise.
---
gcc/jit/jit-playback.c | 24 ++----------------------
gcc/vec.h | 39 +++++++++++++++++++++++++++------------
2 files changed, 29 insertions(+), 34 deletions(-)
diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
index 258ebe8ef86..01c4567de05 100644
--- a/gcc/jit/jit-playback.c
+++ b/gcc/jit/jit-playback.c
@@ -1749,26 +1749,6 @@ block (function *func,
m_label_expr = NULL;
}
-/* A subclass of auto_vec <char *> that frees all of its elements on
- deletion. */
-
-class auto_argvec : public auto_vec <char *>
-{
- public:
- ~auto_argvec ();
-};
-
-/* auto_argvec's dtor, freeing all contained strings, automatically
- chaining up to ~auto_vec <char *>, which frees the internal buffer. */
-
-auto_argvec::~auto_argvec ()
-{
- int i;
- char *str;
- FOR_EACH_VEC_ELT (*this, i, str)
- free (str);
-}
-
/* Compile a playback::context:
- Use the context's options to cconstruct command-line options, and
@@ -1822,7 +1802,7 @@ compile ()
/* Acquire the JIT mutex and set "this" as the active playback ctxt. */
acquire_mutex ();
- auto_argvec fake_args;
+ auto_string_vec fake_args;
make_fake_args (&fake_args, ctxt_progname, &requested_dumps);
if (errors_occurred ())
{
@@ -2440,7 +2420,7 @@ invoke_driver (const char *ctxt_progname,
/* Currently this lumps together both assembling and linking into
TV_ASSEMBLE. */
auto_timevar assemble_timevar (get_timer (), tv_id);
- auto_argvec argvec;
+ auto_string_vec argvec;
#define ADD_ARG(arg) argvec.safe_push (xstrdup (arg))
ADD_ARG (gcc_driver_name);
diff --git a/gcc/vec.h b/gcc/vec.h
index 2d1f468ca1c..905985c41b0 100644
--- a/gcc/vec.h
+++ b/gcc/vec.h
@@ -1447,19 +1447,11 @@ public:
~auto_vec () { this->release (); }
};
-
-/* Allocate heap memory for pointer V and create the internal vector
- with space for NELEMS elements. If NELEMS is 0, the internal
- vector is initialized to empty. */
-
-template<typename T>
-inline void
-vec_alloc (vec<T> *&v, unsigned nelems CXX_MEM_STAT_INFO)
+class auto_string_vec : public auto_vec <char *>
{
- v = new vec<T>;
- v->create (nelems PASS_MEM_STAT);
-}
-
+ public:
+ ~auto_string_vec ();
+};
/* Conditionally allocate heap memory for VEC and its internal vector. */
@@ -1553,6 +1545,29 @@ vec<T, va_heap, vl_ptr>::iterate (unsigned ix, T **ptr) const
vec_safe_iterate ((V), (I), &(P)); \
(I)--)
+/* auto_string_vec's dtor, freeing all contained strings, automatically
+ chaining up to ~auto_vec <char *>, which frees the internal buffer. */
+
+inline
+auto_string_vec::~auto_string_vec ()
+{
+ int i;
+ char *str;
+ FOR_EACH_VEC_ELT (*this, i, str)
+ free (str);
+}
+
+/* Allocate heap memory for pointer V and create the internal vector
+ with space for NELEMS elements. If NELEMS is 0, the internal
+ vector is initialized to empty. */
+
+template<typename T>
+inline void
+vec_alloc (vec<T> *&v, unsigned nelems CXX_MEM_STAT_INFO)
+{
+ v = new vec<T>;
+ v->create (nelems PASS_MEM_STAT);
+}
/* Return a copy of this vector. */
--
2.16.3
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/3] Refactoring to opt-suggestions.[ch].
2018-05-14 12:48 ` Martin Liška
2018-05-14 12:51 ` [PATCH 1/3] Introduce auto_string_vec class Martin Liška
@ 2018-05-14 12:51 ` Martin Liška
2018-06-20 14:57 ` David Malcolm
2018-05-14 12:54 ` [PATCH 3/3] Come up with new --completion option Martin Liška
` (2 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Martin Liška @ 2018-05-14 12:51 UTC (permalink / raw)
To: David Malcolm, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 84 bytes --]
Second part refactors function from gcc.c into a new class
option_proposer.
Martin
[-- Attachment #2: 0002-Refactoring-to-opt-suggestions.-ch.patch --]
[-- Type: text/x-patch, Size: 15640 bytes --]
From c42bb9072242ab44884a71effee9398c6bcf998e Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Mon, 14 May 2018 11:47:08 +0200
Subject: [PATCH 2/3] Refactoring to opt-suggestions.[ch].
gcc/ChangeLog:
2018-05-14 Martin Liska <mliska@suse.cz>
* Makefile.in: Add opt-suggestions.o.
* gcc-main.c: Include opt-suggestions.h.
* gcc.c (driver::driver): Likewise.
(driver::~driver): Remove m_option_suggestions.
(driver::build_option_suggestions): Moved to option_proposer.
(driver::suggest_option): Likewise.
(driver::handle_unrecognized_options): Use option_proposer.
* gcc.h (class driver): Add new memver m_option_proposer.
* opt-suggestions.c: New file.
* opt-suggestions.h: New file.
gcc/c-family/ChangeLog:
2018-05-14 Martin Liska <mliska@suse.cz>
* cppspec.c: Include opt-suggestions.h.
gcc/fortran/ChangeLog:
2018-05-14 Martin Liska <mliska@suse.cz>
* gfortranspec.c: Include opt-suggestions.h.
gcc/jit/ChangeLog:
2018-05-14 Martin Liska <mliska@suse.cz>
* jit-playback.c: Include opt-suggestions.h.
---
gcc/Makefile.in | 2 +-
gcc/c-family/cppspec.c | 1 +
gcc/fortran/gfortranspec.c | 1 +
gcc/gcc-main.c | 1 +
gcc/gcc.c | 114 ++-------------------------------------
gcc/gcc.h | 4 +-
gcc/jit/jit-playback.c | 1 +
gcc/opt-suggestions.c | 129 +++++++++++++++++++++++++++++++++++++++++++++
gcc/opt-suggestions.h | 59 +++++++++++++++++++++
9 files changed, 197 insertions(+), 115 deletions(-)
create mode 100644 gcc/opt-suggestions.c
create mode 100644 gcc/opt-suggestions.h
diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 8ec0511704d..52d485cf36f 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-suggestions.o
# 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..66540239f53 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-suggestions.h"
#include "gcc.h"
#include "opts.h"
diff --git a/gcc/fortran/gfortranspec.c b/gcc/fortran/gfortranspec.c
index fe1ec0447b3..4ba3a8dba60 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-suggestions.h"
#include "gcc.h"
#include "opts.h"
diff --git a/gcc/gcc-main.c b/gcc/gcc-main.c
index 9e6de743adc..6a759bbcc1c 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-suggestions.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..08e76c92df4 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-suggestions.h"
#include "gcc.h"
#include "diagnostic.h"
#include "flags.h"
@@ -7262,8 +7263,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 +7272,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. */
@@ -7768,106 +7760,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 +7768,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;
};
/* The mapping of a spec function name to the C function that
diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
index 01c4567de05..f11642bf4c6 100644
--- a/gcc/jit/jit-playback.c
+++ b/gcc/jit/jit-playback.c
@@ -36,6 +36,7 @@ along with GCC; see the file COPYING3. If not see
#include "attribs.h"
#include "context.h"
#include "fold-const.h"
+#include "opt-suggestions.h"
#include "gcc.h"
#include "diagnostic.h"
diff --git a/gcc/opt-suggestions.c b/gcc/opt-suggestions.c
new file mode 100644
index 00000000000..e322fcd91c5
--- /dev/null
+++ b/gcc/opt-suggestions.c
@@ -0,0 +1,129 @@
+/* Provide option suggestion for --complete option and a misspelled
+ used by a user.
+ Copyright (C) 2016-2018 Free Software Foundation, Inc.
+
+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-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;
+ }
+}
+
+const char *
+option_proposer::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);
+}
+
+void
+option_proposer::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;
+ }
+ }
+}
diff --git a/gcc/opt-suggestions.h b/gcc/opt-suggestions.h
new file mode 100644
index 00000000000..5e3ee9e2671
--- /dev/null
+++ b/gcc/opt-suggestions.h
@@ -0,0 +1,59 @@
+/* Provide suggestions to handle misspelled options, and implement the
+ --complete option for auto-completing options from a prefix.
+ Copyright (C) 2016-2018 Free Software Foundation, Inc.
+
+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)
+ {}
+
+ /* 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);
+
+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 ();
+
+private:
+ /* Cache with all suggestions. */
+ auto_vec <char *> *m_option_suggestions;
+};
+
+#endif /* GCC_OPT_PROPOSER_H */
--
2.16.3
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] Come up with new --completion option.
2018-05-14 12:48 ` Martin Liška
2018-05-14 12:51 ` [PATCH 1/3] Introduce auto_string_vec class Martin Liška
2018-05-14 12:51 ` [PATCH 2/3] Refactoring to opt-suggestions.[ch] Martin Liška
@ 2018-05-14 12:54 ` Martin Liška
2018-06-20 15:27 ` David Malcolm
2018-05-14 13:44 ` RFC: bash completion Martin Liška
2018-06-20 11:55 ` Martin Liška
4 siblings, 1 reply; 19+ messages in thread
From: Martin Liška @ 2018-05-14 12:54 UTC (permalink / raw)
To: David Malcolm, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 119 bytes --]
Main part where I still need to write ChangeLog file and
gcc.sh needs to be moved to bash-completions project.
Martin
[-- Attachment #2: 0003-Come-up-with-new-completion-option.patch --]
[-- Type: text/x-patch, Size: 18401 bytes --]
From cb544b3136559eb3710790dcd582d7bbf36d3792 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Mon, 14 May 2018 11:49:52 +0200
Subject: [PATCH 3/3] Come up with new --completion option.
---
gcc.sh | 51 +++++++++
gcc/common.opt | 4 +
gcc/gcc.c | 15 +++
gcc/opt-suggestions.c | 290 +++++++++++++++++++++++++++++++++++++++++++++--
gcc/opt-suggestions.h | 15 ++-
gcc/opts.c | 3 +
gcc/selftest-run-tests.c | 1 +
gcc/selftest.c | 33 ++++++
gcc/selftest.h | 21 ++++
9 files changed, 418 insertions(+), 15 deletions(-)
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 d6ef85928f3..c57edfb9c94 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
+Provide bash completion for options starting with provided string.
+
-coverage
Driver Alias(coverage)
diff --git a/gcc/gcc.c b/gcc/gcc.c
index 08e76c92df4..c02c2d73b12 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
@@ -3819,6 +3823,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;
@@ -7292,6 +7301,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 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;
- }
-}
-
const char *
option_proposer::suggest_option (const char *bad_opt)
{
@@ -54,6 +42,58 @@ option_proposer::suggest_option (const char *bad_opt)
(auto_vec <const char *> *) m_option_suggestions);
}
+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 parameters. */
+ 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));
+ }
+ }
+}
+
+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)
{
@@ -127,3 +167,229 @@ option_proposer::build_option_suggestions (void)
}
}
}
+
+void
+option_proposer::find_param_completions (const char separator,
+ const char *option_prefix,
+ auto_string_vec &results)
+{
+ char separator_str[] {separator, '\0'};
+ 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);
+ 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 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 ();
-
/* Helper function for driver::handle_unrecognized_options.
Given an unrecognized option BAD_OPT (without the leading dash),
@@ -45,12 +42,24 @@ public:
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 OPTION_PREFIX. */
+ void suggest_completion (const char *option_prefix);
+
+ /* Build completions that start with OPTION_PREFIX and save them
+ into RESULTS vector. */
+ 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 *option_prefix,
+ auto_string_vec &results);
+
private:
/* Cache with all suggestions. */
auto_vec <char *> *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..c68205d3e62 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,17 @@ extern int num_passes;
(HAYSTACK), (NEEDLE)); \
SELFTEST_END_STMT
+/* Evaluate STRING and PREFIX and use strstr to determine if STRING
+ starts with PREFIX.
+ ::selftest::pass if starts.
+ ::selftest::fail if does not start. */
+
+#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. */
--
2.16.3
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: RFC: bash completion
2018-05-14 12:48 ` Martin Liška
` (2 preceding siblings ...)
2018-05-14 12:54 ` [PATCH 3/3] Come up with new --completion option Martin Liška
@ 2018-05-14 13:44 ` Martin Liška
2018-06-20 11:55 ` Martin Liška
4 siblings, 0 replies; 19+ messages in thread
From: Martin Liška @ 2018-05-14 13:44 UTC (permalink / raw)
To: David Malcolm, gcc-patches
On 05/14/2018 02:41 PM, Martin Liška wrote:
> Yes, I will work on that as soon as GCC's part is ready.
Hi.
I've just done branch of bash-completion where I implemented that:
https://github.com/marxin/bash-completion/tree/gcc-completion
Martin
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: RFC: bash completion
2018-05-14 12:48 ` Martin Liška
` (3 preceding siblings ...)
2018-05-14 13:44 ` RFC: bash completion Martin Liška
@ 2018-06-20 11:55 ` Martin Liška
4 siblings, 0 replies; 19+ messages in thread
From: Martin Liška @ 2018-06-20 11:55 UTC (permalink / raw)
To: David Malcolm, gcc-patches
PING^1 (for the whole series).
On 05/14/2018 02:41 PM, Martin Liška wrote:
> On 04/26/2018 01:13 AM, David Malcolm wrote:
>> [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?
>
> Hi David.
>
> Thanks a lot for so detailed review feedback. If not said otherwise I adapted
> all your suggestions.
>
>>
>> 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?
>
> Yes, I will work on that as soon as GCC's part is ready.
>
>>
>>>>
>>>> * 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.
>
> Will do that once we'll make a conclusion about the patches.
>
>>
>> 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)
>
> Done.
>
>>
>>> # 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)
>
> Now it's very bad, due to flat header files one needs to do the explicit inclusion
> of prerequisites for a header file. Don't ask me why..
>
>>
>>>
>>> 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.
>
> There are multiple situations where I have comment in header file (opt-suggestions.h).
> Is it needed to copy it to implementation? I consider it a duplicate information.
>
> I'll send 3 patches that are split from the original one afterwards.
>
> Thanks a lot,
> Martin
>
>>
>>> + 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
>>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] Introduce auto_string_vec class.
2018-05-14 12:51 ` [PATCH 1/3] Introduce auto_string_vec class Martin Liška
@ 2018-06-20 14:41 ` David Malcolm
2018-06-22 11:06 ` Martin Liška
0 siblings, 1 reply; 19+ messages in thread
From: David Malcolm @ 2018-06-20 14:41 UTC (permalink / raw)
To: Martin Liška, gcc-patches
On Mon, 2018-05-14 at 14:50 +0200, Martin Liška wrote:
> First part with introduction of auto_string_vec class.
>
FWIW, I'm fine with the changes to the jit subdir, but I don't think I
have approval rights on the vec.h changes.
BTW, was the move of vec_alloc in vec.h intentional? (I take it that
it's the same before/after, but it seems to have added some churn to
the vec.h part of the patch).
Dave
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] Refactoring to opt-suggestions.[ch].
2018-05-14 12:51 ` [PATCH 2/3] Refactoring to opt-suggestions.[ch] Martin Liška
@ 2018-06-20 14:57 ` David Malcolm
2018-06-22 11:07 ` Martin Liška
0 siblings, 1 reply; 19+ messages in thread
From: David Malcolm @ 2018-06-20 14:57 UTC (permalink / raw)
To: Martin Liška, gcc-patches
On Mon, 2018-05-14 at 14:51 +0200, Martin Liška wrote:
> Second part refactors function from gcc.c into a new class
> option_proposer.
>
> Martin
[...snip...]
diff --git a/gcc/c-family/cppspec.c b/gcc/c-family/cppspec.c
index 1e0a8bcd294..66540239f53 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-suggestions.h"
#include "gcc.h"
I'm not a reviewer for this, and I don't know what our policy here is,
but please can we make "gcc.h" self-contained, and have it include this
the header it needs, rather than have every file that includes "gcc.h"
need to know what includes gcc.h might need. Seems like a violation of
the DRY principle.
[...snip more examples of added headers...]
[...snip changes to gcc.c and gcc.h which I can't approve but which
look sane to me...]
[...snip more examples of added headers...]
diff --git a/gcc/opt-suggestions.c b/gcc/opt-suggestions.c
[...snip...]
+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;
+ }
+}
Could m_option_suggestions be a
auto_string_vec *m_option_suggestions;
rather than a:
auto_vec <char *> *m_option_suggestions;
?
If so, then this dtor's body could simply be:
option_proposer::~option_proposer ()
{
delete m_option_suggestions;
}
[...snip...]
Everything else looks sane to me (I'm not a reviewer, but I
wrote much of the code you're touching).
Dave
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] Come up with new --completion option.
2018-05-14 12:54 ` [PATCH 3/3] Come up with new --completion option Martin Liška
@ 2018-06-20 15:27 ` David Malcolm
2018-06-22 11:25 ` Martin Liška
0 siblings, 1 reply; 19+ messages in thread
From: David Malcolm @ 2018-06-20 15:27 UTC (permalink / raw)
To: Martin Liška, gcc-patches
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] Introduce auto_string_vec class.
2018-06-20 14:41 ` David Malcolm
@ 2018-06-22 11:06 ` Martin Liška
2018-06-22 16:16 ` Jeff Law
0 siblings, 1 reply; 19+ messages in thread
From: Martin Liška @ 2018-06-22 11:06 UTC (permalink / raw)
To: David Malcolm, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 592 bytes --]
On 06/20/2018 04:41 PM, David Malcolm wrote:
> On Mon, 2018-05-14 at 14:50 +0200, Martin Liška wrote:
>> First part with introduction of auto_string_vec class.
>>
>
> FWIW, I'm fine with the changes to the jit subdir, but I don't think I
> have approval rights on the vec.h changes.
Hi.
Good, I'll wait a review from a global reviewer.
>
> BTW, was the move of vec_alloc in vec.h intentional? (I take it that
> it's the same before/after, but it seems to have added some churn to
> the vec.h part of the patch).
It's not intentional, fixed in attached patch.
Martin
>
> Dave
>
[-- Attachment #2: 0001-Introduce-auto_string_vec-class.patch --]
[-- Type: text/x-patch, Size: 3117 bytes --]
From abd1b31ec6807d101bbf868acfbdf3bd02319463 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Mon, 14 May 2018 14:00:07 +0200
Subject: [PATCH] Introduce auto_string_vec class.
gcc/ChangeLog:
2018-05-14 Martin Liska <mliska@suse.cz>
* vec.h (class auto_string_vec): New (moved from auto_argvec).
(auto_string_vec::~auto_string_vec): Likewise.
gcc/jit/ChangeLog:
2018-05-14 Martin Liska <mliska@suse.cz>
* jit-playback.c (class auto_argvec): Moved to vec.h.
(auto_argvec::~auto_argvec): Likewise.
(compile): Use the renamed name.
(invoke_driver): Likewise.
---
gcc/jit/jit-playback.c | 24 ++----------------------
gcc/vec.h | 21 +++++++++++++++++++++
2 files changed, 23 insertions(+), 22 deletions(-)
diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
index 258ebe8ef86..01c4567de05 100644
--- a/gcc/jit/jit-playback.c
+++ b/gcc/jit/jit-playback.c
@@ -1749,26 +1749,6 @@ block (function *func,
m_label_expr = NULL;
}
-/* A subclass of auto_vec <char *> that frees all of its elements on
- deletion. */
-
-class auto_argvec : public auto_vec <char *>
-{
- public:
- ~auto_argvec ();
-};
-
-/* auto_argvec's dtor, freeing all contained strings, automatically
- chaining up to ~auto_vec <char *>, which frees the internal buffer. */
-
-auto_argvec::~auto_argvec ()
-{
- int i;
- char *str;
- FOR_EACH_VEC_ELT (*this, i, str)
- free (str);
-}
-
/* Compile a playback::context:
- Use the context's options to cconstruct command-line options, and
@@ -1822,7 +1802,7 @@ compile ()
/* Acquire the JIT mutex and set "this" as the active playback ctxt. */
acquire_mutex ();
- auto_argvec fake_args;
+ auto_string_vec fake_args;
make_fake_args (&fake_args, ctxt_progname, &requested_dumps);
if (errors_occurred ())
{
@@ -2440,7 +2420,7 @@ invoke_driver (const char *ctxt_progname,
/* Currently this lumps together both assembling and linking into
TV_ASSEMBLE. */
auto_timevar assemble_timevar (get_timer (), tv_id);
- auto_argvec argvec;
+ auto_string_vec argvec;
#define ADD_ARG(arg) argvec.safe_push (xstrdup (arg))
ADD_ARG (gcc_driver_name);
diff --git a/gcc/vec.h b/gcc/vec.h
index a9f3bcf09eb..0af5187782e 100644
--- a/gcc/vec.h
+++ b/gcc/vec.h
@@ -1462,6 +1462,15 @@ vec_alloc (vec<T> *&v, unsigned nelems CXX_MEM_STAT_INFO)
}
+/* A subclass of auto_vec <char *> that frees all of its elements on
+ deletion. */
+
+class auto_string_vec : public auto_vec <char *>
+{
+ public:
+ ~auto_string_vec ();
+};
+
/* Conditionally allocate heap memory for VEC and its internal vector. */
template<typename T>
@@ -1554,6 +1563,18 @@ vec<T, va_heap, vl_ptr>::iterate (unsigned ix, T **ptr) const
vec_safe_iterate ((V), (I), &(P)); \
(I)--)
+/* auto_string_vec's dtor, freeing all contained strings, automatically
+ chaining up to ~auto_vec <char *>, which frees the internal buffer. */
+
+inline
+auto_string_vec::~auto_string_vec ()
+{
+ int i;
+ char *str;
+ FOR_EACH_VEC_ELT (*this, i, str)
+ free (str);
+}
+
/* Return a copy of this vector. */
--
2.17.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] Refactoring to opt-suggestions.[ch].
2018-06-20 14:57 ` David Malcolm
@ 2018-06-22 11:07 ` Martin Liška
2018-06-22 16:09 ` Jeff Law
0 siblings, 1 reply; 19+ messages in thread
From: Martin Liška @ 2018-06-22 11:07 UTC (permalink / raw)
To: David Malcolm, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2040 bytes --]
On 06/20/2018 04:57 PM, David Malcolm wrote:
> On Mon, 2018-05-14 at 14:51 +0200, Martin Liška wrote:
>> Second part refactors function from gcc.c into a new class
>> option_proposer.
>>
>> Martin
>
> [...snip...]
>
> diff --git a/gcc/c-family/cppspec.c b/gcc/c-family/cppspec.c
> index 1e0a8bcd294..66540239f53 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-suggestions.h"
> #include "gcc.h"
>
> I'm not a reviewer for this, and I don't know what our policy here is,
> but please can we make "gcc.h" self-contained, and have it include this
> the header it needs, rather than have every file that includes "gcc.h"
> need to know what includes gcc.h might need. Seems like a violation of
> the DRY principle.
You mean here including "opt-suggestions.h" in "gcc.h"? I believe we have
still the rule to not include a header from header file :/
>
> [...snip more examples of added headers...]
>
> [...snip changes to gcc.c and gcc.h which I can't approve but which
> look sane to me...]
>
> [...snip more examples of added headers...]
>
> diff --git a/gcc/opt-suggestions.c b/gcc/opt-suggestions.c
>
> [...snip...]
>
> +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;
> + }
> +}
>
> Could m_option_suggestions be a
> auto_string_vec *m_option_suggestions;
> rather than a:
> auto_vec <char *> *m_option_suggestions;
Yes.
> ?
>
> If so, then this dtor's body could simply be:
>
> option_proposer::~option_proposer ()
> {
> delete m_option_suggestions;
> }
Sure.
>
> [...snip...]
>
> Everything else looks sane to me (I'm not a reviewer, but I
> wrote much of the code you're touching).
>
> Dave
>
I'm attaching updated version.
Martin
[-- Attachment #2: 0001-Refactoring-to-opt-suggestions.-ch.patch --]
[-- Type: text/x-patch, Size: 15463 bytes --]
From c76e1f0913e3b925516a79ade51a37f496bd2081 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Mon, 14 May 2018 11:47:08 +0200
Subject: [PATCH] Refactoring to opt-suggestions.[ch].
gcc/ChangeLog:
2018-05-14 Martin Liska <mliska@suse.cz>
* Makefile.in: Add opt-suggestions.o.
* gcc-main.c: Include opt-suggestions.h.
* gcc.c (driver::driver): Likewise.
(driver::~driver): Remove m_option_suggestions.
(driver::build_option_suggestions): Moved to option_proposer.
(driver::suggest_option): Likewise.
(driver::handle_unrecognized_options): Use option_proposer.
* gcc.h (class driver): Add new memver m_option_proposer.
* opt-suggestions.c: New file.
* opt-suggestions.h: New file.
gcc/c-family/ChangeLog:
2018-05-14 Martin Liska <mliska@suse.cz>
* cppspec.c: Include opt-suggestions.h.
gcc/fortran/ChangeLog:
2018-05-14 Martin Liska <mliska@suse.cz>
* gfortranspec.c: Include opt-suggestions.h.
gcc/jit/ChangeLog:
2018-05-14 Martin Liska <mliska@suse.cz>
* jit-playback.c: Include opt-suggestions.h.
---
gcc/Makefile.in | 2 +-
gcc/c-family/cppspec.c | 1 +
gcc/fortran/gfortranspec.c | 1 +
gcc/gcc-main.c | 1 +
gcc/gcc.c | 114 +---------------------------------
gcc/gcc.h | 4 +-
gcc/jit/jit-playback.c | 1 +
gcc/opt-suggestions.c | 122 +++++++++++++++++++++++++++++++++++++
gcc/opt-suggestions.h | 59 ++++++++++++++++++
9 files changed, 190 insertions(+), 115 deletions(-)
create mode 100644 gcc/opt-suggestions.c
create mode 100644 gcc/opt-suggestions.h
diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index d8f3e886118..4cc184415fa 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1613,7 +1613,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-suggestions.o
# 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..66540239f53 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-suggestions.h"
#include "gcc.h"
#include "opts.h"
diff --git a/gcc/fortran/gfortranspec.c b/gcc/fortran/gfortranspec.c
index fe1ec0447b3..4ba3a8dba60 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-suggestions.h"
#include "gcc.h"
#include "opts.h"
diff --git a/gcc/gcc-main.c b/gcc/gcc-main.c
index 9e6de743adc..6a759bbcc1c 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-suggestions.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 405d2e38d44..dda1fd35398 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-suggestions.h"
#include "gcc.h"
#include "diagnostic.h"
#include "flags.h"
@@ -7270,8 +7271,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);
}
@@ -7280,14 +7280,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. */
@@ -7776,106 +7768,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
@@ -7884,7 +7776,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;
};
/* The mapping of a spec function name to the C function that
diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
index 01c4567de05..f11642bf4c6 100644
--- a/gcc/jit/jit-playback.c
+++ b/gcc/jit/jit-playback.c
@@ -36,6 +36,7 @@ along with GCC; see the file COPYING3. If not see
#include "attribs.h"
#include "context.h"
#include "fold-const.h"
+#include "opt-suggestions.h"
#include "gcc.h"
#include "diagnostic.h"
diff --git a/gcc/opt-suggestions.c b/gcc/opt-suggestions.c
new file mode 100644
index 00000000000..9e162a6d59d
--- /dev/null
+++ b/gcc/opt-suggestions.c
@@ -0,0 +1,122 @@
+/* Provide option suggestion for --complete option and a misspelled
+ used by a user.
+ Copyright (C) 2016-2018 Free Software Foundation, Inc.
+
+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-suggestions.h"
+#include "selftest.h"
+
+option_proposer::~option_proposer ()
+{
+ delete m_option_suggestions;
+}
+
+const char *
+option_proposer::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);
+}
+
+void
+option_proposer::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;
+ }
+ }
+}
diff --git a/gcc/opt-suggestions.h b/gcc/opt-suggestions.h
new file mode 100644
index 00000000000..ccd4e45a474
--- /dev/null
+++ b/gcc/opt-suggestions.h
@@ -0,0 +1,59 @@
+/* Provide suggestions to handle misspelled options, and implement the
+ --complete option for auto-completing options from a prefix.
+ Copyright (C) 2016-2018 Free Software Foundation, Inc.
+
+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)
+ {}
+
+ /* 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);
+
+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 ();
+
+private:
+ /* Cache with all suggestions. */
+ auto_string_vec *m_option_suggestions;
+};
+
+#endif /* GCC_OPT_PROPOSER_H */
--
2.17.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] Come up with new --completion option.
2018-06-20 15:27 ` David Malcolm
@ 2018-06-22 11:25 ` Martin Liška
2018-06-22 13:19 ` David Malcolm
2018-06-22 16:10 ` Jeff Law
0 siblings, 2 replies; 19+ messages in thread
From: Martin Liška @ 2018-06-22 11:25 UTC (permalink / raw)
To: David Malcolm, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 6250 bytes --]
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
>
[-- Attachment #2: 0001-Come-up-with-new-completion-option.patch --]
[-- Type: text/x-patch, Size: 19392 bytes --]
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. */
--
2.17.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] Come up with new --completion option.
2018-06-22 11:25 ` Martin Liška
@ 2018-06-22 13:19 ` David Malcolm
2018-06-22 16:10 ` Jeff Law
1 sibling, 0 replies; 19+ messages in thread
From: David Malcolm @ 2018-06-22 13:19 UTC (permalink / raw)
To: Martin Liška, gcc-patches
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] Refactoring to opt-suggestions.[ch].
2018-06-22 11:07 ` Martin Liška
@ 2018-06-22 16:09 ` Jeff Law
0 siblings, 0 replies; 19+ messages in thread
From: Jeff Law @ 2018-06-22 16:09 UTC (permalink / raw)
To: Martin Liška, David Malcolm, gcc-patches
On 06/22/2018 05:07 AM, Martin Liška wrote:
> On 06/20/2018 04:57 PM, David Malcolm wrote:
>> On Mon, 2018-05-14 at 14:51 +0200, Martin Liška wrote:
>>> Second part refactors function from gcc.c into a new class
>>> option_proposer.
>>>
>>> Martin
>>
>> [...snip...]
>>
>> diff --git a/gcc/c-family/cppspec.c b/gcc/c-family/cppspec.c
>> index 1e0a8bcd294..66540239f53 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-suggestions.h"
>> #include "gcc.h"
>>
>> I'm not a reviewer for this, and I don't know what our policy here is,
>> but please can we make "gcc.h" self-contained, and have it include this
>> the header it needs, rather than have every file that includes "gcc.h"
>> need to know what includes gcc.h might need. Seems like a violation of
>> the DRY principle.
>
> You mean here including "opt-suggestions.h" in "gcc.h"? I believe we have
> still the rule to not include a header from header file :/
>
>>
>> [...snip more examples of added headers...]
>>
>> [...snip changes to gcc.c and gcc.h which I can't approve but which
>> look sane to me...]
>>
>> [...snip more examples of added headers...]
>>
>> diff --git a/gcc/opt-suggestions.c b/gcc/opt-suggestions.c
>>
>> [...snip...]
>>
>> +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;
>> + }
>> +}
>>
>> Could m_option_suggestions be a
>> auto_string_vec *m_option_suggestions;
>> rather than a:
>> auto_vec <char *> *m_option_suggestions;
>
> Yes.
>
>> ?
>>
>> If so, then this dtor's body could simply be:
>>
>> option_proposer::~option_proposer ()
>> {
>> delete m_option_suggestions;
>> }
>
> Sure.
>
>>
>> [...snip...]
>>
>> Everything else looks sane to me (I'm not a reviewer, but I
>> wrote much of the code you're touching).
>>
>> Dave
>>
>
> I'm attaching updated version.
Based on David's comments, this is fine once the set as a whole is ACK'd.
jeff
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] Come up with new --completion option.
2018-06-22 11:25 ` Martin Liška
2018-06-22 13:19 ` David Malcolm
@ 2018-06-22 16:10 ` Jeff Law
2018-06-28 7:05 ` Martin Liška
1 sibling, 1 reply; 19+ messages in thread
From: Jeff Law @ 2018-06-22 16:10 UTC (permalink / raw)
To: Martin Liška, David Malcolm, gcc-patches
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] Introduce auto_string_vec class.
2018-06-22 11:06 ` Martin Liška
@ 2018-06-22 16:16 ` Jeff Law
0 siblings, 0 replies; 19+ messages in thread
From: Jeff Law @ 2018-06-22 16:16 UTC (permalink / raw)
To: Martin Liška, David Malcolm, gcc-patches
On 06/22/2018 05:06 AM, Martin Liška wrote:
> On 06/20/2018 04:41 PM, David Malcolm wrote:
>> On Mon, 2018-05-14 at 14:50 +0200, Martin Liška wrote:
>>> First part with introduction of auto_string_vec class.
>>>
>> FWIW, I'm fine with the changes to the jit subdir, but I don't think I
>> have approval rights on the vec.h changes.
> Hi.
>
> Good, I'll wait a review from a global reviewer.
>
>> BTW, was the move of vec_alloc in vec.h intentional? (I take it that
>> it's the same before/after, but it seems to have added some churn to
>> the vec.h part of the patch).
> It's not intentional, fixed in attached patch.
>
> Martin
>
>> Dave
>>
>
> 0001-Introduce-auto_string_vec-class.patch
>
>
> From abd1b31ec6807d101bbf868acfbdf3bd02319463 Mon Sep 17 00:00:00 2001
> From: marxin <mliska@suse.cz>
> Date: Mon, 14 May 2018 14:00:07 +0200
> Subject: [PATCH] Introduce auto_string_vec class.
>
> gcc/ChangeLog:
>
> 2018-05-14 Martin Liska <mliska@suse.cz>
>
> * vec.h (class auto_string_vec): New (moved from auto_argvec).
> (auto_string_vec::~auto_string_vec): Likewise.
>
> gcc/jit/ChangeLog:
>
> 2018-05-14 Martin Liska <mliska@suse.cz>
>
> * jit-playback.c (class auto_argvec): Moved to vec.h.
> (auto_argvec::~auto_argvec): Likewise.
> (compile): Use the renamed name.
> (invoke_driver): Likewise.
OK
jeff
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] Come up with new --completion option.
2018-06-22 16:10 ` Jeff Law
@ 2018-06-28 7:05 ` Martin Liška
2018-06-29 8:09 ` Martin Liška
0 siblings, 1 reply; 19+ messages in thread
From: Martin Liška @ 2018-06-28 7:05 UTC (permalink / raw)
To: Jeff Law, David Malcolm, gcc-patches
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] Come up with new --completion option.
2018-06-28 7:05 ` Martin Liška
@ 2018-06-29 8:09 ` Martin Liška
0 siblings, 0 replies; 19+ messages in thread
From: Martin Liška @ 2018-06-29 8:09 UTC (permalink / raw)
To: Jeff Law, David Malcolm, gcc-patches
Hi.
I would like to link bash-completion pull request that adjusts gcc option provides:
https://github.com/scop/bash-completion/pull/222
Martin
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-06-29 7:47 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <3c2e7f32-438a-57a3-96a8-3e9d3fc7f605@suse.cz>
[not found] ` <1524587261.5688.193.camel@redhat.com>
[not found] ` <cabd9b8d-d5a4-b4e4-a28a-07f020a3288a@suse.cz>
2018-04-26 1:28 ` RFC: bash completion David Malcolm
2018-05-14 12:48 ` Martin Liška
2018-05-14 12:51 ` [PATCH 1/3] Introduce auto_string_vec class Martin Liška
2018-06-20 14:41 ` David Malcolm
2018-06-22 11:06 ` Martin Liška
2018-06-22 16:16 ` Jeff Law
2018-05-14 12:51 ` [PATCH 2/3] Refactoring to opt-suggestions.[ch] Martin Liška
2018-06-20 14:57 ` David Malcolm
2018-06-22 11:07 ` Martin Liška
2018-06-22 16:09 ` Jeff Law
2018-05-14 12:54 ` [PATCH 3/3] Come up with new --completion option Martin Liška
2018-06-20 15:27 ` David Malcolm
2018-06-22 11:25 ` Martin Liška
2018-06-22 13:19 ` David Malcolm
2018-06-22 16:10 ` Jeff Law
2018-06-28 7:05 ` Martin Liška
2018-06-29 8:09 ` Martin Liška
2018-05-14 13:44 ` RFC: bash completion Martin Liška
2018-06-20 11:55 ` Martin Liška
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).