On Thu, 2020-03-19 at 14:52 +0100, Martin Liška wrote:
> Hi.
Hi Martin.
> The patch is about basic hint support for -fdump-foo options where
> one can newly get something like:
>
> $ ./xgcc -B. /tmp/foo.c -fdump-ipa-ynline -c
> cc1: error: unrecognized command-line option ‘-fdump-ipa-ynline’; did you
> mean ‘-fdump-ipa-inline’?
> $ ./xgcc -B. /tmp/foo.c -fdump-tree-switchlowe -c
> cc1: error: unrecognized command-line option ‘-fdump-tree-switchlowe’; did
> you mean ‘-fdump-tree-switchlower1’?
> $ ./xgcc -B. /tmp/foo.c -fdump-rtl-sched -c
> cc1: error: unrecognized command-line option ‘-fdump-rtl-sched’; did you mean
> ‘-fdump-rtl-sched1’?
>
> I also considered the same support for --completion option but it's more
> problematic
> as the --completion is handled in driver. In driver we do not instantiate
> pass_manager
> where we have listed all passes (and their corresponding dump options).
This seems like a reasonable restriction; instantiating passes doesn't
seem like something we should be doing from within the driver (what
about things like dumps for arch-specific passes?)
Thinking aloud: would it make sense to move --completion to the
compiler to get around this?
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed in next stage1?
> Thanks,
> Martin
>
> gcc/ChangeLog:
>
> 2020-03-19 Martin Liska <mli...@suse.cz>
>
> * dumpfile.c (dump_switch_p): Change return type
> and print option suggestion.
Strictly speaking this is gcc::dump_manager::dump_switch_p.
[...]
> -int
> +void
> gcc::dump_manager::
> dump_switch_p (const char *arg)
> {
> @@ -1896,8 +1897,22 @@ dump_switch_p (const char *arg)
> for (i = 0; i < m_extra_dump_files_in_use; i++)
> any |= dump_switch_p_1 (arg, &m_extra_dump_files[i], true);
>
> -
> - return any;
> + if (!any)
> + {
> + char *s;
> + auto_vec<const char *> candidates;
> + for (size_t i = TDI_none + 1; i != TDI_end; i++)
> + candidates.safe_push (dump_files[i].swtch);
> + for (size_t i = 0; i < m_extra_dump_files_in_use; i++)
> + candidates.safe_push (m_extra_dump_files[i].swtch);
If I'm reading this right, this covers the simplest cases, but misses
various valid options.
You did indeed describe it as "basic hint support", so fair enough, I
suppose.
What about the glob and "-all" variants?
Also, parse_dump_option does various things here.
Should this be integrated into option_proposer in opt-suggestions.c?
But maybe that's overkill if we're not going to do completion.
> + const char *hint = candidates_list_and_hint (arg, s, candidates);
> + if (hint)
> + error ("unrecognized command-line option %<-fdump-%s%>; "
> + "did you mean %<-fdump-%s%>?", arg, hint);
> + else
> + error ("unrecognized command-line option %<-fdump-%s%>", arg);
> + XDELETEVEC (s);
> + }
> }
candidates_list_and_hint builds a list of space-separated candidates
and writes it back to s (for showing to the user). The above code
ignores this, which is probably good given that everything has an
implicit prefix of "-fdump-" which wouldn't be in those listed strings.
I don't like the way the patch is building that string and then doing
nothing with it. At first I thought it would be cleaner to convert
candidates_list_and_hint's "char *&str" param to a "char **out_str",
and support it being NULL, in which case not to build the candidatate
list.
But even simpler would be to use spellcheck.h's:
extern const char *
find_closest_string (const char *target,
const auto_vec<const char *> *candidates);
to get the hint without building the candidate list.
Why not just do that?
A random other thought: the spellcheck code makes use of overall
lengths to decide if a candidate is a meaningful suggestion, and I
wonder if it's an issue that we're implicitly stripping off and then
re-adding that "-fdump-" prefix. Perhaps not an issue.
[...]
> diff --git a/gcc/testsuite/gcc.dg/spellcheck-options-22.c
> b/gcc/testsuite/gcc.dg/spellcheck-options-22.c
> new file mode 100644
> index 00000000000..b0ddae2e78e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/spellcheck-options-22.c
> @@ -0,0 +1,3 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fdump-ipa-ynline" } */
> +/* { dg-error "unrecognized command-line option '-fdump-ipa-ynline'; did you
> mean '-fdump-ipa-inline'?" "" { target *-*-* } 0 } */
There are various ways in which gcc::dump_manager::dump_switch_p's
"any" can be set, so it would be good to have test coverage of each of
them - but I'm not sure how feasible that is.
What about the "-all" variants?
Maybe that's overkill though.
Hope this is constructive
Dave
