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

[Jakub Jelinek <jakub@redhat.com>]

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 ( ?

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.

	Jakub