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