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