RFC: bash completion
Martin Liška
mliska@suse.cz
Mon May 14 12:48:00 GMT 2018
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
>
More information about the Gcc-patches
mailing list