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;
> 
> 
From ec1dfdc15d1703a3148ecdd269f6dfd4a1b7a40b Mon Sep 17 00:00:00 2001
From: Martin Liska <mli...@suse.cz>
Date: Wed, 11 May 2022 16:07:25 +0200
Subject: [PATCH] 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.

	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  | 24 +++++++++++++++++++++---
 2 files changed, 30 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..3706de38a9d 100644
--- a/gcc/spellcheck.h
+++ b/gcc/spellcheck.h
@@ -128,11 +128,29 @@ 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 that inserts a trailing '=',
+	   so that for
+	   "-ftrivial-auto-var-init"
+	   we suggest
+	   "-ftrivial-auto-var-init="
+	   rather than
+	   "-Wtrivial-auto-var-init".  */
+	/* 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;
+      }
+
+    if (is_better)
       {
 	m_best_distance = dist;
 	m_best_candidate = candidate;
-- 
2.36.1

Reply via email to