This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH 0/2] v3: C/C++: don't suggest implementation names as spelling fixes (PR c/83236)


On Fri, 2017-12-01 at 19:09 -0500, David Malcolm wrote:
> On Fri, 2017-12-01 at 22:56 +0100, Jakub Jelinek wrote:
> > On Fri, Dec 01, 2017 at 04:48:20PM -0500, David Malcolm wrote:
> > > PR c/83236 reports an issue where the C FE unhelpfully suggests
> > > the
> > > use
> > > of glibc's private "__ino_t" type when it fails to recognize
> > > "ino_t":
> > > 
> > > $ cat > test.c <<EOF
> > > #include <sys/stat.h>
> > > ino_t inode;
> > > EOF
> > > $ gcc -std=c89 -fsyntax-only test.c
> > > test.c:2:1: error: unknown type name 'ino_t'; did you mean
> > > '__ino_t'?
> > >  ino_t inode;
> > >  ^~~~~
> > >  __ino_t
> > > 
> > > This patch updates the C/C++ FEs suggestions for unrecognized
> > > identifiers
> > > so that they don't suggest names that are reserved for use by the
> > > implementation i.e. those that begin with an underscore and
> > > either
> > > an
> > > uppercase letter or another underscore.
> > > 
> > > However, it allows built-in macros that match this pattern to be
> > > suggested, since it's useful to be able to suggest __FILE__,
> > > __LINE__
> > > etc.  Other macros *are* filtered.
> > > 
> > > One wart with the patch: the existing macro-handling spellcheck
> > > code
> > > is in spellcheck-tree.c, and needs to call the the new function
> > > "name_reserved_for_implementation_p", however the latter relates
> > > to
> > > the C family of FEs.
> > > Perhaps I should move all of the the macro-handling stuff in
> > > spellcheck-tree.h/c to e.g. a new c-family/c-spellcheck.h/c as a
> > > first step?
> > > 
> > > Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
> > > 
> > > OK for trunk?
> > > 
> > > gcc/c/ChangeLog:
> > > 	PR c/83236
> > > 	* c-decl.c (lookup_name_fuzzy): Don't suggest names that are
> > > 	reserved for use by the implementation.
> > > 
> > > gcc/cp/ChangeLog:
> > > 	PR c/83236
> > > 	* name-lookup.c (consider_binding_level): Don't suggest names
> > > that
> > > 	are reserved for use by the implementation.
> > > 
> > > gcc/ChangeLog:
> > > 	PR c/83236
> > > 	* spellcheck-tree.c (name_reserved_for_implementation_p): New
> > > 	function.
> > > 	(should_suggest_as_macro_p): New function.
> > > 	(find_closest_macro_cpp_cb): Move the check for NT_MACRO to
> > > 	should_suggest_as_macro_p and call it.
> > > 	(selftest::test_name_reserved_for_implementation_p): New
> > > function.
> > > 	(selftest::spellcheck_tree_c_tests): Call it.
> > > 	* spellcheck-tree.h (name_reserved_for_implementation_p): New
> > > 	decl.
> > > 
> > > gcc/testsuite/ChangeLog:
> > > 	PR c/83236
> > > 	* c-c++-common/spellcheck-reserved.c: New test case.
> > > ---
> > >  gcc/c/c-decl.c                                   |  5 +++
> > >  gcc/cp/name-lookup.c                             | 18 +++++++---
> > >  gcc/spellcheck-tree.c                            | 46
> > > +++++++++++++++++++++++-
> > >  gcc/spellcheck-tree.h                            |  2 ++
> > >  gcc/testsuite/c-c++-common/spellcheck-reserved.c | 25
> > > +++++++++++++
> > >  5 files changed, 91 insertions(+), 5 deletions(-)
> > >  create mode 100644 gcc/testsuite/c-c++-common/spellcheck-
> > > reserved.c
> > > 
> > > diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
> > > index 56c63d8..dfd136d 100644
> > > --- a/gcc/c/c-decl.c
> > > +++ b/gcc/c/c-decl.c
> > > @@ -4041,6 +4041,11 @@ lookup_name_fuzzy (tree name, enum
> > > lookup_name_fuzzy_kind kind, location_t loc)
> > >  	if (TREE_CODE (binding->decl) == FUNCTION_DECL)
> > >  	  if (C_DECL_IMPLICIT (binding->decl))
> > >  	    continue;
> > > +	/* Don't suggest names that are reserved for use by the
> > > +	   implementation.  */
> > > +	if (name_reserved_for_implementation_p
> > > +	    (IDENTIFIER_POINTER (binding->id)))
> > 
> > Can't you use a temporary to avoid wrapping line between function
> > name and ( ?
> 
> Fixed.
> 
> > More importantly, does this mean if I mistype __builtin_strtchr it
> > won't suggest __builtin_strrchr?  Would be nice if the filtering
> > of the names reserved for implementation isn't done if the
> > name being looked up is reserved for implementation.
> 
> Good idea, thanks.
> 
> Here's an updated version of the patch.
> 
> Changed in v2:
> * don't filter suggestions if the name name being looked up
>   is itself reserved for implementation
> * fix wrapping in c-decl.c's lookup_name_fuzzy
> * name-lookup.c (consider_binding_level): rename new variable from
> "name"
>   to "suggestion" to avoid shadowing a param
> * spellcheck-tree.c (test_name_reserved_for_implementation_p): Add
> more
>   test coverage ("_" and "__")
> 
> One additional wart I noticed writing the testase is that the
> C and C++ frontends offer different suggestions for
> "__builtin_strtchr".
> C recomends:
>   __builtin_strchr
> whereas C++ recommends:
>   __builtin_strrchr
> 
> The reason is that the C FE visits the builtins in order of
> builtins.def,
> whereas C++ visits them in the reverse order.
> 
> Both have the same edit distance, and so the first "winner" in
> best_match varies between FEs.
> 
> This is a pre-existing issue, though (not sure if it warrants a PR).
> 
> Bootstrap&regrtest in progress; OK if it passes?
> 
> As before, the other wart is that the existing macro-handling
> spellcheck code is in spellcheck-tree.c, and needs to call the the
> new function "name_reserved_for_implementation_p", however the latter
> relates to the C family of FEs.
> Perhaps I should move all of the the macro-handling stuff in
> spellcheck-tree.h/c to e.g. a new c-family/c-spellcheck.h/c as a
> first step?

..and here's a version of the patch which fixes that, also.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.

OK for trunk?

David Malcolm (2):
  Move macro-spellchecking code from "gcc" to new files in c-family
  v3: C/C++: don't suggest implementation names as spelling fixes (PR
    c/83236)

 gcc/Makefile.in                                  |   2 +-
 gcc/c-family/c-common.c                          |   1 +
 gcc/c-family/c-common.h                          |   1 +
 gcc/c-family/c-spellcheck.cc                     | 121 +++++++++++++++++++++++
 gcc/c-family/c-spellcheck.h                      |  53 ++++++++++
 gcc/c/c-decl.c                                   |  12 +++
 gcc/cp/name-lookup.c                             |  25 ++++-
 gcc/spellcheck-tree.c                            |  30 ------
 gcc/spellcheck-tree.h                            |  26 -----
 gcc/testsuite/c-c++-common/spellcheck-reserved.c |  35 +++++++
 10 files changed, 245 insertions(+), 61 deletions(-)
 create mode 100644 gcc/c-family/c-spellcheck.cc
 create mode 100644 gcc/c-family/c-spellcheck.h
 create mode 100644 gcc/testsuite/c-c++-common/spellcheck-reserved.c

-- 
1.8.5.3


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]