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?

Hope this is constructive
Dave

> +
> +    if (is_better)
>        {
>         m_best_distance = dist;
>         m_best_candidate = candidate;


Reply via email to