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