opts: improve option suggestion

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

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;
  

On Wed, May 11, 2022 at 8:50 PM David Malcolm via Gcc-patches
<gcc-patches@gcc.gnu.org> 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;
>
>
  

On 5/11/22 20:49, David Malcolm 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".  */

I welcome the comment suggestion.

> 
> Is the logic correct?  It's comparing the candidate with the goal,
> rather than with the current best.

Yes, that's basically saying that there's a difference in the trailing sign char.

> What if both the candidate and the
> current best both add a trailing equal sign?

Then there's a difference somewhere else, and we would follow edit_distance_t.

> 
> 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 one can't have empty string here.

Sending updated version of the patch where I added the comment.

Ready to be installed?
Thanks,
Martin

> 
> Hope this is constructive
> Dave
> 
>> +
>> +    if (is_better)
>>        {
>>         m_best_distance = dist;
>>         m_best_candidate = candidate;
> 
>
  

PING^1

On 5/12/22 09:10, Martin Liška wrote:
> On 5/11/22 20:49, David Malcolm 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".  */
> 
> I welcome the comment suggestion.
> 
>>
>> Is the logic correct?  It's comparing the candidate with the goal,
>> rather than with the current best.
> 
> Yes, that's basically saying that there's a difference in the trailing sign char.
> 
>> What if both the candidate and the
>> current best both add a trailing equal sign?
> 
> Then there's a difference somewhere else, and we would follow edit_distance_t.
> 
>>
>> 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 one can't have empty string here.
> 
> Sending updated version of the patch where I added the comment.
> 
> Ready to be installed?
> Thanks,
> Martin
> 
>>
>> Hope this is constructive
>> Dave
>>
>>> +
>>> +    if (is_better)
>>>        {
>>>         m_best_distance = dist;
>>>         m_best_candidate = candidate;
>>
>>
  

On 5/24/2022 2:45 AM, Martin Liška wrote:
> PING^1
>
> On 5/12/22 09:10, Martin Liška wrote:
>> On 5/11/22 20:49, David Malcolm 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.
OK
jeff