diff mbox series

v4: C/C++: add fix-it hints for missing '&' and '*' (PR c++/87850)

Message ID 1543621685-20785-1-git-send-email-dmalcolm@redhat.com
State New
Headers show
Series v4: C/C++: add fix-it hints for missing '&' and '*' (PR c++/87850) | expand

Commit Message

David Malcolm Nov. 30, 2018, 11:48 p.m. UTC 
On Tue, 2018-11-20 at 22:23 +0000, Joseph Myers wrote:
> On Tue, 20 Nov 2018, David Malcolm wrote:
> 
> > Should I do:
> 
> You should do whatever is appropriate for the warning in
> question.  But if 
> what's appropriate for the warning in question includes types that
> are 
> compatible but not the same, the comments need to avoid saying it's
> about 
> the types being the same. 

Thanks.

Here's an updated version of the patch.  I added a new
  compatible_types_for_indirection_note_p
specifically for use by maybe_emit_indirection_note, and provided
implementations for C and C++.

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

OK for trunk?


Changed in v4: introduce a compatible_types_for_indirection_note_p
function, and use it in place of same_type_p.

Changed in v3: use same_type_p rather than pointer equality.
Provide implementation of same_type_p for C.

Changed in v2: add a note, and put the fix-it hint on that instead

Blurb follows:

This patch adds a note with a fix-it hint to various
pointer-vs-non-pointer diagnostics, suggesting the addition of
a leading '&' or '*'.

For example, note the ampersand fix-it hint in the following:

demo.c: In function 'int main()':
demo.c:5:22: error: invalid conversion from 'pthread_key_t' {aka 'unsigned int'}
   to 'pthread_key_t*' {aka 'unsigned int*'} [-fpermissive]
    5 |   pthread_key_create(key, NULL);
      |                      ^~~
      |                      |
      |                      pthread_key_t {aka unsigned int}
demo.c:5:22: note: possible fix: take the address with '&'
    5 |   pthread_key_create(key, NULL);
      |                      ^~~
      |                      &
In file included from demo.c:1:
/usr/include/pthread.h:1122:47: note:   initializing argument 1 of
   'int pthread_key_create(pthread_key_t*, void (*)(void*))'
 1122 | extern int pthread_key_create (pthread_key_t *__key,
      |                                ~~~~~~~~~~~~~~~^~~~~

gcc/c-family/ChangeLog:
	PR c++/87850
	* c-common.c: Include "gcc-rich-location.h".
	(maybe_emit_indirection_note): New function.
	* c-common.h (maybe_emit_indirection_note): New decl.
	(compatible_types_for_indirection_note_p): New decl.

gcc/c/ChangeLog:
	PR c++/87850
	* c-typeck.c (compatible_types_for_indirection_note_p): New
	function.
	(convert_for_assignment): Call maybe_emit_indirection_note for
	pointer vs non-pointer diagnostics.

gcc/cp/ChangeLog:
	PR c++/87850
	* call.c (convert_like_real): Call
	maybe_emit_indirection_note for "invalid conversion" diagnostic.
	* typeck.c (compatible_types_for_indirection_note_p): New
	function.

gcc/testsuite/ChangeLog:
	PR c++/87850
	* c-c++-common/indirection-fixits.c: New test.
---
 gcc/c-family/c-common.c                         |  35 +++
 gcc/c-family/c-common.h                         |   4 +
 gcc/c/c-typeck.c                                |  18 +-
 gcc/cp/call.c                                   |   2 +
 gcc/cp/typeck.c                                 |   8 +
 gcc/testsuite/c-c++-common/indirection-fixits.c | 299 ++++++++++++++++++++++++
 6 files changed, 364 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/indirection-fixits.c

Comments

Jason Merrill Dec. 1, 2018, 6:38 p.m. UTC | #1 
On 11/30/18 6:48 PM, David Malcolm wrote:
> On Tue, 2018-11-20 at 22:23 +0000, Joseph Myers wrote:
>> On Tue, 20 Nov 2018, David Malcolm wrote:
>>
>>> Should I do:
>>
>> You should do whatever is appropriate for the warning in question.  But if
>> what's appropriate for the warning in question includes types that are
>> compatible but not the same, the comments need to avoid saying it's about
>> the types being the same.

> Thanks.
> 
> Here's an updated version of the patch.  I added a new
>    compatible_types_for_indirection_note_p
> specifically for use by maybe_emit_indirection_note, and provided
> implementations for C and C++.

> +/* C implementation of compatible_types_for_indirection_note_p.  */
> +
> +bool
> +compatible_types_for_indirection_note_p (tree type1, tree type2)
> +{
> +  return comptypes (type1, type2) == 1;
> +}

Hmm, it looks like the C front-end comptypes will return 1 for e.g. enum 
and int.  It seems to me that what you want for this warning is actually 
to check for the same type.  Perhaps you want to use 
comptypes_check_different_types?  Joseph would know better what's 
correct for the C front-end.

Jason
Joseph Myers Dec. 3, 2018, 10:13 p.m. UTC | #2 
On Sat, 1 Dec 2018, Jason Merrill wrote:

> Hmm, it looks like the C front-end comptypes will return 1 for e.g. enum and
> int.  It seems to me that what you want for this warning is actually to check
> for the same type.  Perhaps you want to use comptypes_check_different_types?
> Joseph would know better what's correct for the C front-end.

Well, it's valid to pass a pointer to enum where a pointer to the 
compatible integer type is required, or vice versa, but I don't have 
advice on which cases you want to accept for this particular fix-it.
Jason Merrill Dec. 5, 2018, 4:03 p.m. UTC | #3 
On Mon, Dec 3, 2018 at 5:14 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Sat, 1 Dec 2018, Jason Merrill wrote:
>
> > Hmm, it looks like the C front-end comptypes will return 1 for e.g. enum and
> > int.  It seems to me that what you want for this warning is actually to check
> > for the same type.  Perhaps you want to use comptypes_check_different_types?
> > Joseph would know better what's correct for the C front-end.
>
> Well, it's valid to pass a pointer to enum where a pointer to the
> compatible integer type is required, or vice versa, but I don't have
> advice on which cases you want to accept for this particular fix-it.

Since the diagnostic is about returning, e.g. an int when an int* is
needed, or vice versa, I don't think considering that kind of
conversion is helpful.

Jason
David Malcolm Dec. 5, 2018, 4:18 p.m. UTC | #4 
On Wed, 2018-12-05 at 11:03 -0500, Jason Merrill wrote:
> On Mon, Dec 3, 2018 at 5:14 PM Joseph Myers <joseph@codesourcery.com>
> wrote:
> > 
> > On Sat, 1 Dec 2018, Jason Merrill wrote:
> > 
> > > Hmm, it looks like the C front-end comptypes will return 1 for
> > > e.g. enum and
> > > int.  It seems to me that what you want for this warning is
> > > actually to check
> > > for the same type.  Perhaps you want to use
> > > comptypes_check_different_types?
> > > Joseph would know better what's correct for the C front-end.
> > 
> > Well, it's valid to pass a pointer to enum where a pointer to the
> > compatible integer type is required, or vice versa, but I don't
> > have
> > advice on which cases you want to accept for this particular fix-
> > it.
> 
> Since the diagnostic is about returning, e.g. an int when an int* is
> needed, or vice versa, I don't think considering that kind of
> conversion is helpful.

FWIW I'd be happy to use simple pointer equality of the types for the C
FE, so that we only emit the suggestion for an exact match (better to
fail to offer a suggestion than to offer a bogus one, I think).

Dave
diff mbox series

Patch

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 9d51815..f235c9d 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -48,6 +48,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "gimplify.h"
 #include "substring-locations.h"
 #include "spellcheck.h"
+#include "gcc-rich-location.h"
 #include "selftest.h"
 
 cpp_reader *parse_in;		/* Declared in c-pragma.h.  */
@@ -8420,6 +8421,40 @@  maybe_suggest_missing_token_insertion (rich_location *richloc,
     }
 }
 
+/* Potentially emit a note about likely missing '&' or '*',
+   depending on EXPR and EXPECTED_TYPE.  */
+
+void
+maybe_emit_indirection_note (location_t loc,
+			     tree expr, tree expected_type)
+{
+  gcc_assert (expr);
+  gcc_assert (expected_type);
+
+  tree actual_type = TREE_TYPE (expr);
+
+  /* Missing '&'.  */
+  if (TREE_CODE (expected_type) == POINTER_TYPE
+      && compatible_types_for_indirection_note_p (actual_type,
+						  TREE_TYPE (expected_type))
+      && lvalue_p (expr))
+    {
+      gcc_rich_location richloc (loc);
+      richloc.add_fixit_insert_before ("&");
+      inform (&richloc, "possible fix: take the address with %qs", "&");
+    }
+
+  /* Missing '*'.  */
+  if (TREE_CODE (actual_type) == POINTER_TYPE
+      && compatible_types_for_indirection_note_p (TREE_TYPE (actual_type),
+						  expected_type))
+    {
+      gcc_rich_location richloc (loc);
+      richloc.add_fixit_insert_before ("*");
+      inform (&richloc, "possible fix: dereference with %qs", "*");
+    }
+}
+
 #if CHECKING_P
 
 namespace selftest {
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index 4187343..0b1fdd7 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -1355,6 +1355,10 @@  extern void maybe_add_include_fixit (rich_location *, const char *, bool);
 extern void maybe_suggest_missing_token_insertion (rich_location *richloc,
 						   enum cpp_ttype token_type,
 						   location_t prev_token_loc);
+extern void maybe_emit_indirection_note (location_t loc,
+					 tree expr, tree expected_type);
+extern bool compatible_types_for_indirection_note_p (tree type1, tree type2);
+
 extern tree braced_list_to_string (tree, tree);
 extern bool has_attribute (location_t, tree, tree, tree (*)(tree));
 
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 81c520a..cb7184b 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -1004,6 +1004,14 @@  common_type (tree t1, tree t2)
   return c_common_type (t1, t2);
 }
 
+/* C implementation of compatible_types_for_indirection_note_p.  */
+
+bool
+compatible_types_for_indirection_note_p (tree type1, tree type2)
+{
+  return comptypes (type1, type2) == 1;
+}
+
 /* Return 1 if TYPE1 and TYPE2 are compatible types for assignment
    or various other operations.  Return 2 if they are compatible
    but a warning may be needed if you use them together.  */
@@ -7293,7 +7301,10 @@  convert_for_assignment (location_t location, location_t expr_loc, tree type,
 	      if (pedwarn (&richloc, OPT_Wint_conversion,
 			   "passing argument %d of %qE makes pointer from "
 			   "integer without a cast", parmnum, rname))
-		inform_for_arg (fundecl, expr_loc, parmnum, type, rhstype);
+		{
+		  maybe_emit_indirection_note (expr_loc, rhs, type);
+		  inform_for_arg (fundecl, expr_loc, parmnum, type, rhstype);
+		}
 	    }
 	    break;
 	  case ic_assign:
@@ -7329,7 +7340,10 @@  convert_for_assignment (location_t location, location_t expr_loc, tree type,
 	    if (pedwarn (&richloc, OPT_Wint_conversion,
 			 "passing argument %d of %qE makes integer from "
 			 "pointer without a cast", parmnum, rname))
-	      inform_for_arg (fundecl, expr_loc, parmnum, type, rhstype);
+	      {
+		maybe_emit_indirection_note (expr_loc, rhs, type);
+		inform_for_arg (fundecl, expr_loc, parmnum, type, rhstype);
+	      }
 	  }
 	  break;
 	case ic_assign:
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index ee099cc..a25d109 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -6862,6 +6862,8 @@  convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
 	  complained = permerror (&richloc,
 				  "invalid conversion from %qH to %qI",
 				  TREE_TYPE (expr), totype);
+	  if (complained)
+	    maybe_emit_indirection_note (loc, expr, totype);
 	}
       if (complained && fn)
 	inform (get_fndecl_argument_location (fn, argnum),
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 81cb405..2730f77 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -1448,6 +1448,14 @@  structural_comptypes (tree t1, tree t2, int strict)
   return comp_type_attributes (t1, t2);
 }
 
+/* C++ implementation of compatible_types_for_indirection_note_p.  */
+
+bool
+compatible_types_for_indirection_note_p (tree type1, tree type2)
+{
+  return same_type_p (type1, type2);
+}
+
 /* Return true if T1 and T2 are related as allowed by STRICT.  STRICT
    is a bitwise-or of the COMPARE_* flags.  */
 
diff --git a/gcc/testsuite/c-c++-common/indirection-fixits.c b/gcc/testsuite/c-c++-common/indirection-fixits.c
new file mode 100644
index 0000000..8987ee9
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/indirection-fixits.c
@@ -0,0 +1,299 @@ 
+/* { dg-options "-fdiagnostics-show-caret" } */
+
+void takes_int_ptr(int*);
+void takes_char_ptr(char*);
+void takes_int(int);
+int returns_int(void);
+
+int ivar;
+char cvar;
+int *int_ptr;
+char *char_ptr;
+
+void test_1 (void)
+{
+  takes_int_ptr(&ivar);
+  takes_int_ptr(int_ptr);
+  takes_char_ptr(&cvar);
+  takes_char_ptr(char_ptr);
+
+  ivar = 42;
+  cvar = 'b';
+  int_ptr = &ivar;
+  char_ptr = &cvar;
+}
+
+void test_2 (void)
+{
+  takes_int_ptr(ivar); /* { dg-warning "" "" { target c } } */
+  /* { dg-error "" "" { target c++ } .-1 } */
+  /* { dg-message "possible fix: take the address with '&'" "" { target *-*-* } .-2 } */
+
+  /* Expect an '&' fix-it hint.  */
+  /* { dg-begin-multiline-output "" }
+   takes_int_ptr(ivar);
+                 ^~~~
+                 |
+                 int
+     { dg-end-multiline-output "" } */
+  /* { dg-begin-multiline-output "" }
+   takes_int_ptr(ivar);
+                 ^~~~
+                 &
+     { dg-end-multiline-output "" } */
+  /* { dg-begin-multiline-output "" }
+ void takes_int_ptr(int*);
+                    ^~~~
+     { dg-end-multiline-output "" } */
+}
+
+void test_3 (void)
+{
+  takes_int_ptr(cvar); /* { dg-warning "" "" { target c } } */
+  /* { dg-error "" "" { target c++ } .-1 } */
+
+  /* Don't expect an '&' fix-it hint.  */
+  /* { dg-begin-multiline-output "" }
+   takes_int_ptr(cvar);
+                 ^~~~
+                 |
+                 char
+     { dg-end-multiline-output "" } */
+  /* { dg-begin-multiline-output "" }
+ void takes_int_ptr(int*);
+                    ^~~~
+     { dg-end-multiline-output "" } */
+}
+
+void test_4 (void)
+{
+  takes_char_ptr(ivar); /* { dg-warning "" "" { target c } } */
+  /* { dg-error "" "" { target c++ } .-1 } */
+
+  /* Don't expect an '&' fix-it hint.  */
+  /* { dg-begin-multiline-output "" }
+   takes_char_ptr(ivar);
+                  ^~~~
+                  |
+                  int
+     { dg-end-multiline-output "" } */
+  /* { dg-begin-multiline-output "" }
+ void takes_char_ptr(char*);
+                     ^~~~~
+     { dg-end-multiline-output "" } */
+}
+
+void test_5 (void)
+{
+  takes_char_ptr(cvar); /* { dg-warning "" "" { target c } } */
+  /* { dg-error "" "" { target c++ } .-1 } */
+
+  /* Expect an '&' fix-it hint.  */
+  /* { dg-begin-multiline-output "" }
+   takes_char_ptr(cvar); 
+                  ^~~~
+                  |
+                  char
+     { dg-end-multiline-output "" } */
+  /* { dg-begin-multiline-output "" }
+   takes_char_ptr(cvar); 
+                  ^~~~
+                  &
+     { dg-end-multiline-output "" } */
+  /* { dg-begin-multiline-output "" }
+ void takes_char_ptr(char*);
+                     ^~~~~
+     { dg-end-multiline-output "" } */
+}
+ 
+void test_6 (void)
+{
+  takes_int(int_ptr); /* { dg-warning "" "" { target c } } */
+  /* { dg-error "" "" { target c++ } .-1 } */
+  /* { dg-message "possible fix: dereference with '*'" "" { target *-*-* } .-2 } */
+
+  /* Expect a '*' fix-it hint.  */
+  /* { dg-begin-multiline-output "" }
+   takes_int(int_ptr);
+             ^~~~~~~
+             |
+             int *
+     { dg-end-multiline-output "" { target c } } */
+  /* { dg-begin-multiline-output "" }
+   takes_int(int_ptr);
+             ^~~~~~~
+             |
+             int*
+     { dg-end-multiline-output "" { target c++ } } */
+  /* { dg-begin-multiline-output "" }
+   takes_int(int_ptr);
+             ^~~~~~~
+             *
+     { dg-end-multiline-output "" } */
+  /* { dg-begin-multiline-output "" }
+ void takes_int(int);
+                ^~~
+     { dg-end-multiline-output "" } */
+}
+ 
+void test_7 (void)
+{
+  takes_int(char_ptr); /* { dg-warning "" "" { target c } } */
+  /* { dg-error "" "" { target c++ } .-1 } */
+
+  /* Don't expect a '*' fix-it hint.  */
+  /* { dg-begin-multiline-output "" }
+   takes_int(char_ptr);
+             ^~~~~~~~
+             |
+             char *
+     { dg-end-multiline-output "" { target c } } */
+  /* { dg-begin-multiline-output "" }
+   takes_int(char_ptr);
+             ^~~~~~~~
+             |
+             char*
+     { dg-end-multiline-output "" { target c++ } } */
+  /* { dg-begin-multiline-output "" }
+ void takes_int(int);
+                ^~~
+     { dg-end-multiline-output "" } */
+}
+ 
+void test_8 (void)
+{
+  ivar = int_ptr; /* { dg-warning "" "" { target c } } */
+  /* { dg-error "" "" { target c++ } .-1 } */
+
+  /* Expect a fix-it hint from the C++ FE, but not from C (due to missing
+     location).  */
+  /* { dg-begin-multiline-output "" }
+   ivar = int_ptr;
+          ^~~~~~~
+          |
+          int*
+     { dg-end-multiline-output "" { target c++ } } */
+  /* { dg-begin-multiline-output "" }
+   ivar = int_ptr;
+          ^~~~~~~
+          *
+     { dg-end-multiline-output "" { target c++ } } */
+  /* { dg-begin-multiline-output "" }
+   ivar = int_ptr;
+        ^
+     { dg-end-multiline-output "" { target c } } */
+}
+
+void test_9 (void)
+{
+  cvar = int_ptr; /* { dg-warning "" "" { target c } } */
+  /* { dg-error "" "" { target c++ } .-1 } */
+
+  /* Don't expect a '*' fix-it hint.  */
+  /* { dg-begin-multiline-output "" }
+   cvar = int_ptr;
+          ^~~~~~~
+          |
+          int*
+     { dg-end-multiline-output "" { target c++ } } */
+  /* { dg-begin-multiline-output "" }
+   cvar = int_ptr;
+        ^
+     { dg-end-multiline-output "" { target c } } */
+}
+
+void test_10 (void)
+{
+  int_ptr = ivar; /* { dg-warning "" "" { target c } } */
+  /* { dg-error "" "" { target c++ } .-1 } */
+
+  /* Expect a fix-it hint from the C++ FE, but not from C (due to missing
+     location).  */
+  /* { dg-begin-multiline-output "" }
+   int_ptr = ivar;
+             ^~~~
+             |
+             int
+     { dg-end-multiline-output "" { target c++ } } */
+  /* { dg-begin-multiline-output "" }
+   int_ptr = ivar;
+             ^~~~
+             &
+     { dg-end-multiline-output "" { target c++ } } */
+  /* { dg-begin-multiline-output "" }
+   int_ptr = ivar;
+           ^
+     { dg-end-multiline-output "" { target c } } */
+}
+
+void test_11 (void)
+{
+  char_ptr = ivar; /* { dg-warning "" "" { target c } } */
+  /* { dg-error "" "" { target c++ } .-1 } */
+
+  /* Don't expect a fix-it hint, due to mismatching types.  */
+  /* { dg-begin-multiline-output "" }
+   char_ptr = ivar;
+              ^~~~
+              |
+              int
+     { dg-end-multiline-output "" { target c++ } } */
+  /* { dg-begin-multiline-output "" }
+   char_ptr = ivar;
+            ^
+     { dg-end-multiline-output "" { target c } } */
+}
+
+/* We shouldn't offer '&' fix-it hints for non-lvalues.  */
+
+void test_12 (void)
+{
+  takes_int_ptr (returns_int ()); /* { dg-warning "" "" { target c } } */
+  /* { dg-error "" "" { target c++ } .-1 } */
+
+  /* { dg-begin-multiline-output "" }
+   takes_int_ptr (returns_int ());
+                  ^~~~~~~~~~~~~~
+                  |
+                  int
+     { dg-end-multiline-output "" { target c } } */
+  /* { dg-begin-multiline-output "" }
+   takes_int_ptr (returns_int ());
+                  ~~~~~~~~~~~~^~
+                              |
+                              int
+     { dg-end-multiline-output "" { target c++ } } */
+  /* { dg-begin-multiline-output "" }
+ void takes_int_ptr(int*);
+                    ^~~~
+     { dg-end-multiline-output "" } */
+}
+
+/* Ignore typedefs when offering fix-it hints.  */
+
+typedef int typedef_int_t;
+typedef_int_t typedef_int_t_var;
+
+void test_13 (void)
+{
+  takes_int_ptr (typedef_int_t_var); /* { dg-warning "" "" { target c } } */
+  /* { dg-error "" "" { target c++ } .-1 } */
+  /* { dg-message "possible fix: take the address with '&'" "" { target *-*-* } .-2 } */
+
+  /* Expect an '&' fix-it hint.  */
+  /* { dg-begin-multiline-output "" }
+   takes_int_ptr (typedef_int_t_var);
+                  ^~~~~~~~~~~~~~~~~
+                  |
+                  typedef_int_t {aka int}
+     { dg-end-multiline-output "" } */
+  /* { dg-begin-multiline-output "" }
+   takes_int_ptr (typedef_int_t_var);
+                  ^~~~~~~~~~~~~~~~~
+                  &
+     { dg-end-multiline-output "" } */
+  /* { dg-begin-multiline-output "" }
+ void takes_int_ptr(int*);
+                    ^~~~
+     { dg-end-multiline-output "" } */
+}