Re: [C PATCH] Ignore invisible bindings for misspelling hints (PR c/71858)

[David Malcolm <dmalcolm at redhat dot com>]

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