On Mon, 2018-09-24 at 10:56 -0600, Martin Sebor wrote:
> On 09/21/2018 04:09 PM, David Malcolm wrote:
> > This is v2 of the patch; I managed to bit-rot my own patch due to
> > my
> > fix for r264335, which tightened up the "is this meaningful"
> > threshold
> > on edit distances when finding spelling correction candidates.
> >
> > The only change in this version is to rename various things in
> > the testcase so that they continue to be suggested
> > ("colour" vs "m_color" are no longer near enough, so I renamed
> > "colour" to "m_colour").
> >
> > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> >
> > OK for trunk?
> >
> > Blurb from v1:
> >
> > PR c++/84993 identifies a problem with our suggestions for
> > misspelled member names in the C++ FE for the case where the
> > member is private.
> >
> > For example, given:
> >
> > class foo
> > {
> > public:
> > double get_ratio() const { return m_ratio; }
> >
> > private:
> > double m_ratio;
> > };
> >
> > void test(foo *ptr)
> > {
> > if (ptr->ratio >= 0.5)
> > ;// etc
> > }
> >
> > ...we currently emit this suggestion:
> >
> > <source>: In function 'void test(foo*)':
> > <source>:12:12: error: 'class foo' has no member named 'ratio'; did
> > you mean 'm_ratio'?
> > if (ptr->ratio >= 0.5)
> > ^~~~~
> > m_ratio
> >
> > ...but if the user follows this suggestion, they get:
> >
> > <source>: In function 'void test(foo*)':
> > <source>:12:12: error: 'double foo::m_ratio' is private within this
> > context
> > if (ptr->m_ratio >= 0.5)
> > ^~~~~~~
> > <source>:7:10: note: declared private here
> > double m_ratio;
> > ^~~~~~~
> > <source>:12:12: note: field 'double foo::m_ratio' can be accessed
> > via 'double foo::get_ratio() const'
> > if (ptr->m_ratio >= 0.5)
> > ^~~~~~~
> > get_ratio()
> >
> > It feels wrong to be emitting a fix-it hint that doesn't compile,
> > so this
> > patch adds the accessor fix-it hint logic to this case, so that we
> > directly
> > offer a valid suggestion:
> >
> > <source>: In function 'void test(foo*)':
> > <source>:12:12: error: 'class foo' has no member named 'ratio'; did
> > you mean
> > 'double foo::m_ratio'? (accessible via 'double foo::get_ratio()
> > const')
> > if (ptr->ratio >= 0.5)
> > ^~~~~
> > get_ratio()
>
> I wonder if suggesting the inaccessible member is at all helpful
> if it cannot be used.
Of the two members "m_ratio" and "get_ratio", the winning candidate
based on edit-distance lookup was the inaccessible member; the accessor
is "further away" in terms of edit distance. I think it's useful to
the user to identify both, as this patch does.
> Would mentioning only the accessor be
> a better approach?
I think it's more helpful to the user to be explicit about what the
compiler is "thinking", and mention both. Consider the case where the
accessor has a wildy different name to the misspelled member:
class foo {
public:
int get_pertinent_data () { return m_val; }
private:
int m_val;
};
int test (foo *f)
{
return f->val;
}
The patch emits:
t.c: In function ‘int test(foo*)’:
t.c:11:14: error: ‘class foo’ has no member named ‘val’; did you mean
‘int foo::m_val’? (accessible via ‘int foo::get_pertinent_data()’)
11 | return f->val;
| ^~~
| get_pertinent_data()
which I think is clear and helpful.
> Also, to echo a comment made by someone else in a bug I don't
> remember, rather than phrasing the error in the form of
> a question ("did you mean x?") it might be better to either
> state what we know:
>
> error: 'class foo' has no member named 'ratio'; the nearest
> match is 'double foo::m_ratio' (accessible via 'double
> foo::get_ratio() const')
>
> or offer a suggestion:
>
> error: 'class foo' has no member named 'ratio'; suggest
> using 'double foo::get_ratio() const' instead
I think you're referring to PR 84890, though that bug was about this
kind of message:
note: ‘INT_MAX’ is defined in header ‘<limits.h>’; did you forget to
‘#include <limits.h>’?
PR 84890 combines several concerns:
(a) is the message too verbose? (initial comment)
(b) is the message potentially annoying to the user? (comment 5)
(c) is it better to phrase the message as a suggestion, rather than as
a question? (comment 8)
Taking these in turn:
(a) may apply to the missing include message, but I think the "did you
mean 'foo'" is short and clear
(b) both styles of message refer to the user as "you", which has a risk
of making things personal. I think it's fine for the "did you mean"
case. I think there's an argument that the "did you forget" could be
annoying - but that's not a concern for this patch.
(c) of the various wordings:
(c.1) "did you mean 'foo'?" (as in the patch, and the status quo for
the rest of the C and C++ FE)
(c.2) "suggest using 'foo' instead" doesn't seem grammatically
correct to me, in that it sounds like the user is being asked to
suggest something, rather than to change their code. I think there's
an implicit "I" in there - "I suggest" - but after abbreviation it
seems clunky to me.
(c.3) "suggestion: use 'foo'" might work, but (c.1) seems much
simpler to me
So I prefer the wording in the patch. If we're going to change the
wording across the compiler, I'd prefer to do that in a separate patch.
> A different approach altogether to these spelling messages that
> occurs to me but that you may have already considered and rejected
> would be to do what GCC does for errors due to ambiguous overloads:
> i.e., enumerate the available candidates. This would work well in
> cases when multiple members are a close match. It would also make
> it possible to explain, for each candidate member, whether it's
> accessible.
I hadn't thought of that approach, but my hunch is that there's likely
to only be one good suggestion: this use-case is trying to handle the
case where the user is trying to access a field of a class, and can't
quite remember the spelling, or what the accessor is.
FWIW I actually have another patch I'm about to post which updates
those available candidates suggestions (it special-cases the common
case where there's just one candidate: it uses the "did you mean %qs?"
wording, and the user gets a fix-it hint inline, and two less "note"
diagnostics)
Dave
