On Wed, May 11, 2022 at 8:50 PM David Malcolm via Gcc-patches
<[email protected]> wrote:
>
> On Wed, 2022-05-11 at 16:49 +0200, Martin Liška wrote:
> > In case where we have 2 equally good candidates like
> > -ftrivial-auto-var-init=
> > -Wtrivial-auto-var-init
> >
> > for -ftrivial-auto-var-init, we should take the candidate that
> > has a difference in trailing sign symbol.
> >
> > Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> >
> > Ready to be installed?
> > Thanks,
> > Martin
> >
> > PR driver/105564
> >
> > gcc/ChangeLog:
> >
> > * spellcheck.cc (test_find_closest_string): Add new test.
> > * spellcheck.h (class best_match): Prefer a difference in
> > trailing sign symbol.
> > ---
> > gcc/spellcheck.cc | 9 +++++++++
> > gcc/spellcheck.h | 17 ++++++++++++++---
> > 2 files changed, 23 insertions(+), 3 deletions(-)
> >
> > diff --git a/gcc/spellcheck.cc b/gcc/spellcheck.cc
> > index 3e58344f510..f728573331f 100644
> > --- a/gcc/spellcheck.cc
> > +++ b/gcc/spellcheck.cc
> > @@ -464,6 +464,15 @@ test_find_closest_string ()
> > ASSERT_STREQ ("DWARF_GNAT_ENCODINGS_ALL",
> > find_closest_string ("DWARF_GNAT_ENCODINGS_all",
> > &candidates));
> > +
> > + /* Example from PR 105564 where option name with missing equal
> > + sign should win. */
> > + candidates.truncate (0);
> > + candidates.safe_push ("-Wtrivial-auto-var-init");
> > + candidates.safe_push ("-ftrivial-auto-var-init=");
> > + ASSERT_STREQ ("-ftrivial-auto-var-init=",
> > + find_closest_string ("-ftrivial-auto-var-init",
> > + &candidates));
> > }
> >
> > /* Test data for test_metric_conditions. */
> > diff --git a/gcc/spellcheck.h b/gcc/spellcheck.h
> > index 9b6223695be..9111ea08fc3 100644
> > --- a/gcc/spellcheck.h
> > +++ b/gcc/spellcheck.h
> > @@ -128,11 +128,22 @@ class best_match
> >
> > /* Otherwise, compute the distance and see if the candidate
> > has beaten the previous best value. */
> > + const char *candidate_str = candidate_traits::get_string
> > (candidate);
> > edit_distance_t dist
> > - = get_edit_distance (m_goal, m_goal_len,
> > - candidate_traits::get_string (candidate),
> > - candidate_len);
> > + = get_edit_distance (m_goal, m_goal_len, candidate_str,
> > candidate_len);
> > +
> > + bool is_better = false;
> > if (dist < m_best_distance)
> > + is_better = true;
> > + else if (dist == m_best_distance)
> > + {
> > + /* Prefer a candidate has a difference in trailing sign
> > character. */
> > + if (candidate_str[candidate_len - 1] == '='
> > + && m_goal[m_goal_len - 1] != '=')
> > + is_better = true;
> > + }
>
> Thanks for working on this.
>
> Maybe the comment should read:
>
> /* Prefer a candidate that inserts a trailing '=',
> so that for
> "-ftrivial-auto-var-init"
> we suggest
> "-ftrivial-auto-var-init="
> rather than
> "-Wtrivial-auto-var-init". */
>
> Is the logic correct? It's comparing the candidate with the goal,
> rather than with the current best. What if both the candidate and the
> current best both add a trailing equal sign?
>
> I find the array access of the final character suspicious - is there
> any chance that either of the lengths could be zero? I don't think so,
> but maybe we should bulletproof things, and move the "is better"
> comparison to a subroutine?
I think for this case the logic would be to prefer to stick to the
-W or -f _prefix_ the user gave and give that a "boost" so
we do not suggest an optimization option for a warning one.
That is, when given -Wfoo we should not suggest -ffoo-bar
but -Wfoo-longbar (just made up example), even if the
-ffoo-bar looks a much better match.
But maybe that's better handled by better selecting the initial
candidate set rather than doing the last disambiguation.
Richard.
> Hope this is constructive
> Dave
>
> > +
> > + if (is_better)
> > {
> > m_best_distance = dist;
> > m_best_candidate = candidate;
>
>
