diff mbox

[3/6] New warnings -Wstring-plus-{char, int} (PR c++/62181)

Message ID 1497231292.27153.11.camel@stu.xidian.edu.cn
State New
Headers show

Commit Message

Xi Ruoyao June 12, 2017, 1:34 a.m. UTC 
This patch adds warning option -Wstring-plus-char for C/C++.

gcc/ChangeLog:

2017-06-12  Xi Ruoyao  <ryxi@stu.xidian.edu.cn>

	* c-family/c-common.h (warn_if_string_plus_char): New prototype.
	* c-family/c-warn.c (warn_if_string_plus_char): New function.
	* c-family/c.opt: New option -Wstring-plus-char.
	* cp/call.c (build_new_op_1): Check for -Wstring-plus-char.
	* c/c-typeck.c (parser_build_binary_op, build_modify_expr):
	Check for -Wstring-plus-char.
---
 gcc/c-family/c-common.h |  1 +
 gcc/c-family/c-warn.c   | 22 ++++++++++++++++++++++
 gcc/c-family/c.opt      |  5 +++++
 gcc/c/c-typeck.c        | 14 ++++++++++++++
 gcc/cp/call.c           | 28 ++++++++++++++++++++++++++++
 5 files changed, 70 insertions(+)

Comments

Martin Sebor June 19, 2017, 4:30 p.m. UTC | #1 
On 06/11/2017 07:34 PM, Xi Ruoyao wrote:
> This patch adds warning option -Wstring-plus-char for C/C++.
>

+void
+warn_if_string_plus_char (location_t loc, tree ptrtype, tree inttype)
+{
+  if (POINTER_TYPE_P (ptrtype)
+      && type_main_variant_is_char (TREE_TYPE (ptrtype))
+      && type_main_variant_is_char (inttype))
+    warning_at (loc, OPT_Wstring_plus_char,
+                "add %qT to string pointer %qT does not append "
+                "to the string", inttype, ptrtype);

The text of the warning doesn't read like a grammatically correct
sentence.  ("Adding a to b" would be correct.)

That said, I wonder if it should also be made more accurate.
Based on c-c++-common/Wstring-plus-char.c for the snippet below

   char *a;
   const char *b;
   const char c = 'c';
   const char *d = a + c;

it will print

   warning: add 'char' to 'char *' does not append to the string

even though no string is apparent or need to exist in the program
(a could point to an array of chars with no terminating NUL).
I see Clang prints something similar (modulo the bad grammar) but
I think it might be clearer if the warning instead read something
like:

   adding 'char' to 'char *' does not append to a string

or (if the warning were to trigger only for character constants
like in Clang):

   adding 'char' to 'char *' does not append 'c' to the first operand

i.e., if the warning also included the value of the character
constant.

Martin
Xi Ruoyao June 19, 2017, 5:35 p.m. UTC | #2 
On 2017-06-19 10:30 -0600, Martin Sebor wrote:
> On 06/11/2017 07:34 PM, Xi Ruoyao wrote:
> > This patch adds warning option -Wstring-plus-char for C/C++.
> > 
> 
> +void
> +warn_if_string_plus_char (location_t loc, tree ptrtype, tree inttype)
> +{
> +  if (POINTER_TYPE_P (ptrtype)
> +      && type_main_variant_is_char (TREE_TYPE (ptrtype))
> +      && type_main_variant_is_char (inttype))
> +    warning_at (loc, OPT_Wstring_plus_char,
> +                "add %qT to string pointer %qT does not append "
> +                "to the string", inttype, ptrtype);
> 
> The text of the warning doesn't read like a grammatically correct
> sentence.  ("Adding a to b" would be correct.)

Yes.  It's a typo.

> That said, I wonder if it should also be made more accurate.
> Based on c-c++-common/Wstring-plus-char.c for the snippet below
> 
>    char *a;
>    const char *b;
>    const char c = 'c';
>    const char *d = a + c;
> 
> it will print
> 
>    warning: add 'char' to 'char *' does not append to the string
> 
> even though no string is apparent or need to exist in the program
> (a could point to an array of chars with no terminating NUL).
> I see Clang prints something similar (modulo the bad grammar) but
> I think it might be clearer if the warning instead read something
> like:
> 
>    adding 'char' to 'char *' does not append to a string
> 
> or (if the warning were to trigger only for character constants
> like in Clang):
> 
>    adding 'char' to 'char *' does not append 'c' to the first operand

Clang 4.0 only warns for character constants.  But Clang 5.0
(pre-release) also warns for variables with type char.
Which option should we take?  Maybe -Wstring-plus-char={1,2} ?

> i.e., if the warning also included the value of the character
> constant.
> 
> Martin
diff mbox

Patch

diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index 82ed4f6..82f1c25 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -1501,6 +1501,7 @@  extern void c_do_switch_warnings (splay_tree, location_t, tree, tree, bool,
 				  bool);
 extern void warn_for_omitted_condop (location_t, tree);
 extern void warn_for_restrict (unsigned, tree *, unsigned);
+extern void warn_if_string_plus_char (location_t, tree, tree);
 
 /* Places where an lvalue, or modifiable lvalue, may be required.
    Used to select diagnostic messages in lvalue_error and
diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c
index 35321a6..d804500 100644
--- a/gcc/c-family/c-warn.c
+++ b/gcc/c-family/c-warn.c
@@ -2332,6 +2332,28 @@  warn_for_restrict (unsigned param_pos, tree *argarray, unsigned nargs)
 			 arg_positions.length ());
 }
 
+/* Like char_type_p, but check the main variant and filter out
+   char16_t (uint_least16_t) and char32_t (uint_least32_t) in C11.  */
+static inline bool
+type_main_variant_is_char (tree type)
+{
+  type = TYPE_MAIN_VARIANT (type);
+  return char_type_p (type)
+         && type != uint_least16_type_node
+         && type != uint_least32_type_node;
+}
+
+void
+warn_if_string_plus_char (location_t loc, tree ptrtype, tree inttype)
+{
+  if (POINTER_TYPE_P (ptrtype)
+      && type_main_variant_is_char (TREE_TYPE (ptrtype))
+      && type_main_variant_is_char (inttype))
+    warning_at (loc, OPT_Wstring_plus_char,
+                "add %qT to string pointer %qT does not append "
+                "to the string", inttype, ptrtype);
+}
+
 /* Callback function to determine whether an expression TP or one of its
    subexpressions comes from macro expansion.  Used to suppress bogus
    warnings.  */
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 94ba3eb..e13cc6c 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -732,6 +732,11 @@  C ObjC C++ ObjC++ Joined RejectNegative UInteger Var(warn_stringop_overflow) Ini
 Under the control of Object Size type, warn about buffer overflow in string
 manipulation functions like memcpy and strcpy.
 
+Wstring-plus-char
+C ObjC C++ ObjC++ Var(warn_string_plus_char) Warning
+Warn about adding string pointers and characters, which is likely an
+ill-formed attempt to append the string.
+
 Wstring-plus-int
 C ObjC C++ ObjC++ Var(warn_string_plus_int) Warning
 Warn about adding strings and integers, which is likely an ill-formed
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 8adfa9a..44e7e02 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -3659,6 +3659,12 @@  parser_build_binary_op (location_t location, enum tree_code code,
 			   arg1.get_start (),
 			   arg2.get_finish ());
 
+  if (warn_string_plus_char && code == PLUS_EXPR)
+    {
+      warn_if_string_plus_char (location, type1, type2);
+      warn_if_string_plus_char (location, type2, type1);
+    }
+
   /* Check for cases such as x+y<<z which users are likely
      to misinterpret.  */
   if (warn_parentheses)
@@ -5788,6 +5794,14 @@  build_modify_expr (location_t location, tree lhs, tree lhs_origtype,
 	  newrhs = build_binary_op (location,
 				    modifycode, lhs, newrhs, 1);
 
+	  if (warn_string_plus_char)
+	    {
+	      tree lt = (lhs_origtype ? lhs_origtype : TREE_TYPE (lhs));
+	      tree rt = (rhs_origtype ? rhs_origtype : TREE_TYPE (rhs));
+	      warn_if_string_plus_char (location, lt, rt);
+	      warn_if_string_plus_char (location, rt, lt);
+	    }
+
 	  /* The original type of the right hand side is no longer
 	     meaningful.  */
 	  rhs_origtype = NULL_TREE;
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index ef99683..8e5ce6a 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -5586,6 +5586,8 @@  build_new_op_1 (location_t loc, enum tree_code code, int flags, tree arg1,
   enum tree_code code2 = NOP_EXPR;
   enum tree_code code_orig_arg1 = ERROR_MARK;
   enum tree_code code_orig_arg2 = ERROR_MARK;
+  tree orig_type1 = error_mark_node;
+  tree orig_type2 = error_mark_node;
   conversion *conv;
   void *p;
   bool strict_p;
@@ -5642,6 +5644,12 @@  build_new_op_1 (location_t loc, enum tree_code code, int flags, tree arg1,
 
       /* =, ->, [], () must be non-static member functions.  */
     case MODIFY_EXPR:
+      if (code2 == PLUS_EXPR)
+        {
+          /* Saved for warn_if_string_plus_char.  */
+          orig_type1 = TREE_TYPE (arg1);
+          orig_type2 = TREE_TYPE (arg2);
+        }
       if (code2 != NOP_EXPR)
 	break;
       /* FALLTHRU */
@@ -5649,6 +5657,11 @@  build_new_op_1 (location_t loc, enum tree_code code, int flags, tree arg1,
     case ARRAY_REF:
       memonly = true;
       break;
+    case PLUS_EXPR:
+      /* Saved for warn_if_string_plus_char.  */
+      orig_type1 = TREE_TYPE (arg1);
+      orig_type2 = TREE_TYPE (arg2);
+      break;
 
     default:
       break;
@@ -5977,6 +5990,13 @@  build_new_op_1 (location_t loc, enum tree_code code, int flags, tree arg1,
   switch (code)
     {
     case MODIFY_EXPR:
+      if (code2 == PLUS_EXPR
+          && (complain & tf_warning)
+          && warn_string_plus_char)
+        {
+          warn_if_string_plus_char (loc, TREE_TYPE (arg1), orig_type2);
+          warn_if_string_plus_char (loc, TREE_TYPE (arg2), orig_type2);
+        }
       return cp_build_modify_expr (loc, arg1, code2, arg2, complain);
 
     case INDIRECT_REF:
@@ -6016,6 +6036,14 @@  build_new_op_1 (location_t loc, enum tree_code code, int flags, tree arg1,
     case BIT_AND_EXPR:
     case BIT_IOR_EXPR:
     case BIT_XOR_EXPR:
+      if ((complain & tf_warning)
+          && warn_string_plus_char
+          && code == PLUS_EXPR)
+        {
+          warn_if_string_plus_char (loc, TREE_TYPE (arg1), orig_type2);
+          warn_if_string_plus_char (loc, TREE_TYPE (arg2), orig_type1);
+        }
+
       return cp_build_binary_op (loc, code, arg1, arg2, complain);
 
     case UNARY_PLUS_EXPR:
-- 
2.7.1