On Thu, 2016-07-14 at 19:47 +0200, Jakub Jelinek wrote:
> On Thu, Jul 14, 2016 at 01:22:27PM -0400, David Malcolm wrote:
> > I wrote a patch for this, similar to yours, but with a slightly
> > different approach for handling builtins.
> >
> > I think it's reasonable to offer suggestions for misspellings of
> > e.g.
> > "snprintf" even if the pertinent header hasn't been included, since
> > it
> > will help guide a novice developer to the missing header. For
> > example,
> > if the user has:
> >
> > void
> > test_7 (int i, int j)
> > {
> > int buffer[100];
> > snprint (buffer, 100, "%i of %i", i, j);
> > }
> >
> > ...then they get:
> >
> > test.c: In function ‘test_7’:
> > test.c:5:3: warning: implicit declaration of function ‘snprint’;
> > did
> > you mean ‘snprintf’? [-Wimplicit-function-declaration]
> > snprint (buffer, 100, "%i of %i", i, j);
> > ^~~~~~~
> > snprintf
> >
> > ...and on correcting it, they get the "you forgot <stdio.h>"
> > warning:
>
> I find it weird. What is special on the builtins? Lots of standard
> headers
> contain lots of functions that aren't backed by builtins, some of
> them used
> more often than the builtins; the reason for builtins is often just
> that gcc
> wants to be able to optimize them somehow.
>
> E.g. note fopen is not a builtin, so you still will not suggest it if
> stdio.h is not included.
>
> Not all the builtins out there are even enabled in all compilation
> modes
> when the containing header is included, it can depend on the language
> version, feature test macros, etc. So, by considering invisible
> builtins
> you could e.g. suggest C99 builtins in strict C89 compilation mode
> where the
> header would not define it, etc.
>
> In any case, this is your baby, so it is up to the FE maintainers and
> you to
> get agreement on, I won't fight on this.
Thanks.
I agree with the points you raise; in that light I'm fine with your
patch.
> > What I find surprising is the suggestion of "carg"; I've seen this
> > a
> > few times as a suggestion. I confess to having to look up carg;
> > IMHO
> > "carg" is much less likely to be a useful suggestion than, say,
> > "printf".
>
> For the short identifiers, the question is if the maximum Damerau
> -Levenshtein
> distance (do you handle transpositions now?)
We don't currently handle transpositions, but it would be good to do so
(PR other/69968).
> shouldn't be lowered (say for at
> most 4 chars identifiers only consider levenshtein distance 1 and not
> more,
> e.g. for 3 character identifiers allowing distance of 2 or 3 means
> you pretty
> much suggest any other 3 char identifier), regardless of if it is a
> builtin or not.
The logic in get_best_meaningful_candidate is currently:
unsigned int cutoff = MAX (m_goal_len, m_best_candidate_len) / 2;
if (m_best_distance > cutoff)
return NULL;
For a pair of 3 character identifiers, cutoff will be 3 / 2 == 1, so it
will suggest within a distance of 1 and reject if the distance is 2 or
3. Or is there a bug?
That cutoff could definitely be improved. Perhaps the MAX should be
changed to a MIN? Or just base it off the goal length e.g.
unsigned int cutoff = m_goal_len / 2;
thus implicitly rejecting
suggestions when m_goal_len == 1.
That would eliminate the "bar" to
"char" suggestion in the PR, since m_goal_len == 3 would have a cutoff
of 1.
>
> > @@ -3992,6 +3995,26 @@ lookup_name_fuzzy (tree name, enum
> > lookup_name_fuzzy_kind kind)
> > if (kind == FUZZY_LOOKUP_TYPENAME)
> > if (TREE_CODE (binding->decl) != TYPE_DECL)
> > continue;
> > + if (kind == FUZZY_LOOKUP_FUNCTION_NAME)
> > + if (TREE_CODE (binding->decl) != FUNCTION_DECL)
> > + continue;
>
> Note this is handled in the second patch I've posted, and this one
> doesn't
> handle function pointer variables/parameters.
>
> Jakub
