[PATCHv2] Call braced_list_to_string after array size is fixed

Hi,

this updated patch fixes one regression with current trunk due
to a new test case.  Sorry for the confusion.

The change to the previous version is:
1) the check to avoid folding on empty char arrays is restored.
2) A null-termination character is added except when the string is full length.


Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.

On 08/24/2018 01:52 PM, Bernd Edlinger wrote:
> Hi,
> 
> this updated patch fixes one regression with current trunk due
> to a new test case.  Sorry for the confusion.
> 
> The change to the previous version is:
> 1) the check to avoid folding on empty char arrays is restored.
> 2) A null-termination character is added except when the string is full length.
> 
> 
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
> 
> 
> Thanks
> Bernd.
> 
> 
> patch-bracedstr-v2.diff
> 
> 
> c-family:
> 2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* c-common.c (braced_list_to_string): Remove eval parameter.
> 	Add some more checks.  Always create zero-terminated STRING_CST.
> 	* c-common.h (braced_list_to_string): Adjust prototype.
> 
> c:
> 2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* c-decl.c (finish_decl): Call braced_list_to_string here ...
> 	* c-parser.c (c_parser_declaration_or_fndef): ... instead of here.
> 
> 
> cp:
> 2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* decl.c (eval_check_narrowing): Remove.
> 	(check_initializer): Move call to braced_list_to_string from here ...
> 	* typeck2.c (store_init_value): ... to here.
> 	(digest_init_r): Remove handing of signed/unsigned char strings.
> 
> testsuite:
> 2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* c-c++-common/array-init.c: New test.
> 	* g++.dg/init/string2.C: Remove xfail.
My concern here is that you removed code that was explicitly added
during the initial review work of the patch that turned braced
initializers into strings.


> -    a[] = { 1, 2, 333, 0 };         // { dg-warning "\\\[\(-Wnarrowing|-Woverflow\)" "" { target { ! c++98_only } } }
> +    a[] = { 1, 2, 333, 0 };         // { dg-warning "\\\[\(-Wnarrowing|-Woverflow\)" }
This is not an XFAIL, this is a selector.  Essentially it says that the
diagnostic is appropriate when not in c++98 mode.

You can see that to be the case if you compile the test in c++98 mode
without your change.  It will compile with no errors or warnings.

However, after your change it issues a warning for the narrowing
conversion in c++98 mode, which AFAICT it should not do.

jeff

On 08/26/18 05:34, Jeff Law wrote:
> On 08/24/2018 01:52 PM, Bernd Edlinger wrote:
>> Hi,
>>
>> this updated patch fixes one regression with current trunk due
>> to a new test case.  Sorry for the confusion.
>>
>> The change to the previous version is:
>> 1) the check to avoid folding on empty char arrays is restored.
>> 2) A null-termination character is added except when the string is full length.
>>
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
>>
>>
>> Thanks
>> Bernd.
>>
>>
>> patch-bracedstr-v2.diff
>>
>>
>> c-family:
>> 2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>> 	* c-common.c (braced_list_to_string): Remove eval parameter.
>> 	Add some more checks.  Always create zero-terminated STRING_CST.
>> 	* c-common.h (braced_list_to_string): Adjust prototype.
>>
>> c:
>> 2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>> 	* c-decl.c (finish_decl): Call braced_list_to_string here ...
>> 	* c-parser.c (c_parser_declaration_or_fndef): ... instead of here.
>>
>>
>> cp:
>> 2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>> 	* decl.c (eval_check_narrowing): Remove.
>> 	(check_initializer): Move call to braced_list_to_string from here ...
>> 	* typeck2.c (store_init_value): ... to here.
>> 	(digest_init_r): Remove handing of signed/unsigned char strings.
>>
>> testsuite:
>> 2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>> 	* c-c++-common/array-init.c: New test.
>> 	* g++.dg/init/string2.C: Remove xfail.
> My concern here is that you removed code that was explicitly added
> during the initial review work of the patch that turned braced
> initializers into strings.
> 

Yes, you mean BRACE_ENCLOSED_INITIALIZER_P
which is (TREE_CODE (NODE) == CONSTRUCTOR && TREE_TYPE (NODE) == init_list_type_node)

I tried that first but the TREE_TYPE of the CONSTRUCTOR was no longer init_list_type_node
at that point.  I think the middle-end needs the structure type here, and digest_init must
have fixed that.

This did not break in the debugger:
+  if (TREE_CODE (type) == ARRAY_TYPE
+      && TREE_CODE (value) == CONSTRUCTOR)
+    {
+      tree typ1 = TYPE_MAIN_VARIANT (TREE_TYPE (type));
+      if (typ1 == char_type_node
+	 || typ1 == signed_char_type_node
+	 || typ1 == unsigned_char_type_node)
+       {
+        if (BRACE_ENCLOSED_INITIALIZER_P(value))
+          asm("int3");
+	value = braced_list_to_string (type, value);
+       }
+    }
+

while this
+        if (!BRACE_ENCLOSED_INITIALIZER_P(value))
+          printf("%p - %p\n", TREE_TYPE(value), init_list_type_node);

does this:

$ cat test.cc
char x[] = {1,2,3};
$ ../gcc-build/gcc/xgcc -B ../gcc-build/gcc/ -S test.cc
0x7fdedb821b28 - 0x7fdedb81f738

while the string is folded as expected.

> 
>> -    a[] = { 1, 2, 333, 0 };         // { dg-warning "\\\[\(-Wnarrowing|-Woverflow\)" "" { target { ! c++98_only } } }
>> +    a[] = { 1, 2, 333, 0 };         // { dg-warning "\\\[\(-Wnarrowing|-Woverflow\)" }
> This is not an XFAIL, this is a selector.  Essentially it says that the
> diagnostic is appropriate when not in c++98 mode.
> 
> You can see that to be the case if you compile the test in c++98 mode
> without your change.  It will compile with no errors or warnings.
> 
> However, after your change it issues a warning for the narrowing
> conversion in c++98 mode, which AFAICT it should not do.
> 

This just restores the state _before_ Martin's braced initializer patch.
So I have the impression that is actually a regression due to the
original braced initializer patch.
It is unfortunate that Martin did not check that.


Bernd.

On 08/24/2018 03:52 PM, Bernd Edlinger wrote:
> this updated patch fixes one regression with current trunk due
> to a new test case.  Sorry for the confusion.
> 
> The change to the previous version is:
> 1) the check to avoid folding on empty char arrays is restored.
> 2) A null-termination character is added except when the string is full length.

> -		      && TYPE_STRING_FLAG (TREE_TYPE (valtype)))

> +      tree typ1 = TYPE_MAIN_VARIANT (TREE_TYPE (type));
> +      if (typ1 == char_type_node
> +	  || typ1 == signed_char_type_node
> +	  || typ1 == unsigned_char_type_node)


Why stop using TYPE_STRING_FLAG?

Jason

On 08/30/18 06:34, Jason Merrill wrote:
> On 08/24/2018 03:52 PM, Bernd Edlinger wrote:
>> this updated patch fixes one regression with current trunk due
>> to a new test case.  Sorry for the confusion.
>>
>> The change to the previous version is:
>> 1) the check to avoid folding on empty char arrays is restored.
>> 2) A null-termination character is added except when the string is full length.
> 
>> -              && TYPE_STRING_FLAG (TREE_TYPE (valtype)))
> 
>> +      tree typ1 = TYPE_MAIN_VARIANT (TREE_TYPE (type));
>> +      if (typ1 == char_type_node
>> +      || typ1 == signed_char_type_node
>> +      || typ1 == unsigned_char_type_node)
> 
> 
> Why stop using TYPE_STRING_FLAG?
> 

No longer sure, I will try it with TYPE_STRING_FLAG.


Bernd.

On 08/30/18 09:07, Bernd Edlinger wrote:
> On 08/30/18 06:34, Jason Merrill wrote:
>> On 08/24/2018 03:52 PM, Bernd Edlinger wrote:
>>> this updated patch fixes one regression with current trunk due
>>> to a new test case.  Sorry for the confusion.
>>>
>>> The change to the previous version is:
>>> 1) the check to avoid folding on empty char arrays is restored.
>>> 2) A null-termination character is added except when the string is full length.
>>
>>> -              && TYPE_STRING_FLAG (TREE_TYPE (valtype)))
>>
>>> +      tree typ1 = TYPE_MAIN_VARIANT (TREE_TYPE (type));
>>> +      if (typ1 == char_type_node
>>> +      || typ1 == signed_char_type_node
>>> +      || typ1 == unsigned_char_type_node)
>>
>>
>> Why stop using TYPE_STRING_FLAG?
>>
> 
> No longer sure, I will try it with TYPE_STRING_FLAG.
> 

This is an update that uses TYPE_STRING_FLAG.
It does not seem to make any difference, though.

Bootstrapped and reg-tested on x86_64-pc-linux-gnu.


Is it OK for trunk?


Thanks
Bernd.
c-family:
2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-common.c (braced_list_to_string): Remove eval parameter.
	Add some more checks.  Always create zero-terminated STRING_CST.
	* c-common.h (braced_list_to_string): Adjust prototype.

c:
2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-decl.c (finish_decl): Call braced_list_to_string here ...
	* c-parser.c (c_parser_declaration_or_fndef): ... instead of here.


cp:
2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* decl.c (eval_check_narrowing): Remove.
	(check_initializer): Move call to braced_list_to_string from here ...
	* typeck2.c (store_init_value): ... to here.
	(digest_init_r): Remove handing of signed/unsigned char strings.

testsuite:
2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-c++-common/array-init.c: New test.
	* g++.dg/init/string2.C: Remove xfail.

diff -Npur gcc/c/c-decl.c gcc/c/c-decl.c
--- gcc/c/c-decl.c	2018-08-21 08:17:41.000000000 +0200
+++ gcc/c/c-decl.c	2018-08-30 12:03:12.508230259 +0200
@@ -5031,6 +5031,12 @@ finish_decl (tree decl, location_t init_
       relayout_decl (decl);
     }
 
+  if (TREE_CODE (type) == ARRAY_TYPE
+      && TYPE_STRING_FLAG (TREE_TYPE (type))
+      && DECL_INITIAL (decl)
+      && TREE_CODE (DECL_INITIAL (decl)) == CONSTRUCTOR)
+    DECL_INITIAL (decl) = braced_list_to_string (type, DECL_INITIAL (decl));
+
   if (VAR_P (decl))
     {
       if (init && TREE_CODE (init) == CONSTRUCTOR)
diff -Npur gcc/c/c-parser.c gcc/c/c-parser.c
--- gcc/c/c-parser.c	2018-08-21 08:17:41.000000000 +0200
+++ gcc/c/c-parser.c	2018-08-24 20:03:12.511230218 +0200
@@ -2127,15 +2127,6 @@ c_parser_declaration_or_fndef (c_parser
 	      if (d != error_mark_node)
 		{
 		  maybe_warn_string_init (init_loc, TREE_TYPE (d), init);
-
-		  /* Try to convert a string CONSTRUCTOR into a STRING_CST.  */
-		  tree valtype = TREE_TYPE (init.value);
-		  if (TREE_CODE (init.value) == CONSTRUCTOR
-		      && TREE_CODE (valtype) == ARRAY_TYPE
-		      && TYPE_STRING_FLAG (TREE_TYPE (valtype)))
-		    if (tree str = braced_list_to_string (valtype, init.value))
-		      init.value = str;
-
 		  finish_decl (d, init_loc, init.value,
 			       init.original_type, asm_name);
 		}
diff -Npur gcc/c-family/c-common.c gcc/c-family/c-common.c
--- gcc/c-family/c-common.c	2018-08-17 05:02:11.000000000 +0200
+++ gcc/c-family/c-common.c	2018-08-24 20:03:12.512230205 +0200
@@ -8511,39 +8511,28 @@ maybe_add_include_fixit (rich_location *
 }
 
 /* Attempt to convert a braced array initializer list CTOR for array
-   TYPE into a STRING_CST for convenience and efficiency.  When non-null,
-   use EVAL to attempt to evalue constants (used by C++).  Return
-   the converted string on success or null on failure.  */
+   TYPE into a STRING_CST for convenience and efficiency.  Return
+   the converted string on success or the original ctor on failure.  */
 
 tree
-braced_list_to_string (tree type, tree ctor, tree (*eval)(tree, tree))
+braced_list_to_string (tree type, tree ctor)
 {
-  unsigned HOST_WIDE_INT nelts = CONSTRUCTOR_NELTS (ctor);
+  if (!tree_fits_uhwi_p (TYPE_SIZE_UNIT (type)))
+    return ctor;
 
   /* If the array has an explicit bound, use it to constrain the size
      of the string.  If it doesn't, be sure to create a string that's
      as long as implied by the index of the last zero specified via
      a designator, as in:
        const char a[] = { [7] = 0 };  */
-  unsigned HOST_WIDE_INT maxelts = HOST_WIDE_INT_M1U;
-  if (tree size = TYPE_SIZE_UNIT (type))
-    {
-      if (tree_fits_uhwi_p (size))
-	{
-	  maxelts = tree_to_uhwi (size);
-	  maxelts /= tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (type)));
+  unsigned HOST_WIDE_INT maxelts = tree_to_uhwi (TYPE_SIZE_UNIT (type));
+  maxelts /= tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (type)));
 
-	  /* Avoid converting initializers for zero-length arrays.  */
-	  if (!maxelts)
-	    return NULL_TREE;
-	}
-    }
-  else if (!nelts)
-    /* Avoid handling the undefined/erroneous case of an empty
-       initializer for an arrays with unspecified bound.  */
-    return NULL_TREE;
+  /* Avoid converting initializers for zero-length arrays.  */
+  if (!maxelts)
+    return ctor;
 
-  tree eltype = TREE_TYPE (type);
+  unsigned HOST_WIDE_INT nelts = CONSTRUCTOR_NELTS (ctor);
 
   auto_vec<char> str;
   str.reserve (nelts + 1);
@@ -8553,19 +8542,21 @@ braced_list_to_string (tree type, tree c
 
   FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (ctor), i, index, value)
     {
-      unsigned HOST_WIDE_INT idx = index ? tree_to_uhwi (index) : i;
+      unsigned HOST_WIDE_INT idx = i;
+      if (index)
+	{
+	  if (!tree_fits_uhwi_p (index))
+	    return ctor;
+	  idx = tree_to_uhwi (index);
+	}
 
       /* auto_vec is limited to UINT_MAX elements.  */
       if (idx > UINT_MAX)
-	return NULL_TREE;
+	return ctor;
 
-      /* Attempt to evaluate constants.  */
-      if (eval)
-	value = eval (eltype, value);
-
-      /* Avoid non-constant initializers.  */
+     /* Avoid non-constant initializers.  */
      if (!tree_fits_shwi_p (value))
-	return NULL_TREE;
+	return ctor;
 
       /* Skip over embedded nuls except the last one (initializer
 	 elements are in ascending order of indices).  */
@@ -8573,11 +8564,14 @@ braced_list_to_string (tree type, tree c
       if (!val && i + 1 < nelts)
 	continue;
 
+      if (idx < str.length())
+	return ctor;
+
       /* Bail if the CTOR has a block of more than 256 embedded nuls
 	 due to implicitly initialized elements.  */
       unsigned nchars = (idx - str.length ()) + 1;
       if (nchars > 256)
-	return NULL_TREE;
+	return ctor;
 
       if (nchars > 1)
 	{
@@ -8585,18 +8579,17 @@ braced_list_to_string (tree type, tree c
 	  str.quick_grow_cleared (idx);
 	}
 
-      if (idx > maxelts)
-	return NULL_TREE;
+      if (idx >= maxelts)
+	return ctor;
 
       str.safe_insert (idx, val);
     }
 
-  if (!nelts)
-    /* Append a nul for the empty initializer { }.  */
+  /* Append a nul string termination.  */
+  if (str.length () < maxelts)
     str.safe_push (0);
 
-  /* Build a STRING_CST with the same type as the array, which
-     may be an array of unknown bound.  */
+  /* Build a STRING_CST with the same type as the array.  */
   tree res = build_string (str.length (), str.begin ());
   TREE_TYPE (res) = type;
   return res;
diff -Npur gcc/c-family/c-common.h gcc/c-family/c-common.h
--- gcc/c-family/c-common.h	2018-08-17 05:02:11.000000000 +0200
+++ gcc/c-family/c-common.h	2018-08-24 20:03:12.512230205 +0200
@@ -1331,7 +1331,7 @@ extern void maybe_add_include_fixit (ric
 extern void maybe_suggest_missing_token_insertion (rich_location *richloc,
 						   enum cpp_ttype token_type,
 						   location_t prev_token_loc);
-extern tree braced_list_to_string (tree, tree, tree (*)(tree, tree) = NULL);
+extern tree braced_list_to_string (tree, tree);
 
 #if CHECKING_P
 namespace selftest {
diff -Npur gcc/cp/decl.c gcc/cp/decl.c
--- gcc/cp/decl.c	2018-08-22 22:35:38.000000000 +0200
+++ gcc/cp/decl.c	2018-08-24 20:03:12.514230178 +0200
@@ -6299,30 +6299,6 @@ build_aggr_init_full_exprs (tree decl, t
   return build_aggr_init (decl, init, flags, tf_warning_or_error);
 }
 
-/* Attempt to determine the constant VALUE of integral type and convert
-   it to TYPE, issuing narrowing warnings/errors as necessary.  Return
-   the constant result or null on failure.  Callback for
-   braced_list_to_string.  */
-
-static tree
-eval_check_narrowing (tree type, tree value)
-{
-  if (tree valtype = TREE_TYPE (value))
-    {
-      if (TREE_CODE (valtype) != INTEGER_TYPE)
-	return NULL_TREE;
-    }
-  else
-    return NULL_TREE;
-
-  value = scalar_constant_value (value);
-  if (!value)
-    return NULL_TREE;
-
-  check_narrowing (type, value, tf_warning_or_error);
-  return value;
-}
-
 /* Verify INIT (the initializer for DECL), and record the
    initialization in DECL_INITIAL, if appropriate.  CLEANUP is as for
    grok_reference_init.
@@ -6438,17 +6414,7 @@ check_initializer (tree decl, tree init,
 	    }
 	  else
 	    {
-	      /* Try to convert a string CONSTRUCTOR into a STRING_CST.  */
-	      tree valtype = TREE_TYPE (decl);
-	      if (TREE_CODE (valtype) == ARRAY_TYPE
-		  && TYPE_STRING_FLAG (TREE_TYPE (valtype))
-		  && BRACE_ENCLOSED_INITIALIZER_P (init))
-		if (tree str = braced_list_to_string (valtype, init,
-						      eval_check_narrowing))
-		  init = str;
-
-	      if (TREE_CODE (init) != STRING_CST)
-		init = reshape_init (type, init, tf_warning_or_error);
+	      init = reshape_init (type, init, tf_warning_or_error);
 	      flags |= LOOKUP_NO_NARROWING;
 	    }
 	}
diff -Npur gcc/cp/typeck2.c gcc/cp/typeck2.c
--- gcc/cp/typeck2.c	2018-08-22 22:35:38.000000000 +0200
+++ gcc/cp/typeck2.c	2018-08-30 12:03:12.515230164 +0200
@@ -807,6 +807,11 @@ store_init_value (tree decl, tree init,
     /* Digest the specified initializer into an expression.  */
     value = digest_init_flags (type, init, flags, tf_warning_or_error);
 
+  if (TREE_CODE (type) == ARRAY_TYPE
+      && TYPE_STRING_FLAG (TREE_TYPE (type))
+      && TREE_CODE (value) == CONSTRUCTOR)
+    value = braced_list_to_string (type, value);
+
   value = extend_ref_init_temps (decl, value, cleanups);
 
   /* In C++11 constant expression is a semantic, not syntactic, property.
@@ -1058,9 +1063,7 @@ digest_init_r (tree type, tree init, int
 
 	  if (TYPE_PRECISION (typ1) == BITS_PER_UNIT)
 	    {
-	      if (char_type != char_type_node
-		  && char_type != signed_char_type_node
-		  && char_type != unsigned_char_type_node)
+	      if (char_type != char_type_node)
 		{
 		  if (complain & tf_error)
 		    error_at (loc, "char-array initialized from wide string");
diff -Npur gcc/testsuite/c-c++-common/array-init.c gcc/testsuite/c-c++-common/array-init.c
--- gcc/testsuite/c-c++-common/array-init.c	1970-01-01 01:00:00.000000000 +0100
+++ gcc/testsuite/c-c++-common/array-init.c	2018-08-24 20:03:12.515230164 +0200
@@ -0,0 +1,4 @@
+/* { dg-do compile } */
+/* { dg-prune-output "sorry, unimplemented: non-trivial designated initializers not supported" } */
+
+char x[] = { [-1] = 1, 2, 3 }; /* { dg-error "array index in initializer exceeds array bounds" "" { target c } } */
diff -Npur gcc/testsuite/g++.dg/init/string2.C gcc/testsuite/g++.dg/init/string2.C
--- gcc/testsuite/g++.dg/init/string2.C	2018-08-16 20:21:59.000000000 +0200
+++ gcc/testsuite/g++.dg/init/string2.C	2018-08-24 20:03:12.515230164 +0200
@@ -54,7 +54,7 @@ template <class T>
 int tmplen ()
 {
   static const T
-    a[] = { 1, 2, 333, 0 };         // { dg-warning "\\\[\(-Wnarrowing|-Woverflow\)" "" { target { ! c++98_only } } }
+    a[] = { 1, 2, 333, 0 };         // { dg-warning "\\\[\(-Wnarrowing|-Woverflow\)" }
   return __builtin_strlen (a);
 }

On 08/26/2018 01:45 AM, Bernd Edlinger wrote:

>>> cp:
>>> 2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>>
>>> 	* decl.c (eval_check_narrowing): Remove.
>>> 	(check_initializer): Move call to braced_list_to_string from here ...
>>> 	* typeck2.c (store_init_value): ... to here.
>>> 	(digest_init_r): Remove handing of signed/unsigned char strings.
>>>
>>> testsuite:
>>> 2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>>
>>> 	* c-c++-common/array-init.c: New test.
>>> 	* g++.dg/init/string2.C: Remove xfail.
>> My concern here is that you removed code that was explicitly added
>> during the initial review work of the patch that turned braced
>> initializers into strings.
>>
> 
> Yes, you mean BRACE_ENCLOSED_INITIALIZER_P
> which is (TREE_CODE (NODE) == CONSTRUCTOR && TREE_TYPE (NODE) == init_list_type_node)
I was referring to the callback into the eval routine from within
braced_list_to_string which is related to the concern where you dropped
the c++98_only selector.  Sorry I wasn't clear about that.

[ snip ]

> 
>>
>>> -    a[] = { 1, 2, 333, 0 };         // { dg-warning "\\\[\(-Wnarrowing|-Woverflow\)" "" { target { ! c++98_only } } }
>>> +    a[] = { 1, 2, 333, 0 };         // { dg-warning "\\\[\(-Wnarrowing|-Woverflow\)" }
>> This is not an XFAIL, this is a selector.  Essentially it says that the
>> diagnostic is appropriate when not in c++98 mode.
>>
>> You can see that to be the case if you compile the test in c++98 mode
>> without your change.  It will compile with no errors or warnings.
>>
>> However, after your change it issues a warning for the narrowing
>> conversion in c++98 mode, which AFAICT it should not do.
>>
> 
> This just restores the state _before_ Martin's braced initializer patch.
> So I have the impression that is actually a regression due to the
> original braced initializer patch.
> It is unfortunate that Martin did not check that.
The ChangeLog said it was removing an xfail, so naturally when I saw it
was removing a selector I assumed that the selector was right
(particularly since it made sense given current behavior) and you'd made
a minor goof.

But I can confirm that prior to Martin's changes we did issue a
diagnostic when in C++98 mode:

j.C: In instantiation of ‘int tmplen() [with T = char]’:
j.C:61:27:   required from here
j.C:57:5: warning: overflow in conversion from ‘int’ to ‘char’ changes
value from ‘333’ to ‘'M'’ [-Woverflow]
57 |     a[] = { 1, 2, 333, 0 };         // { dg-warning
"\\\[\(-Wnarrowing|-Woverflow\)" "" { target { ! c++98_only } } }
   |     ^

You can see that with the trunk just before Martin's change or with
older compilers (I also tested with 6.4.1 as I had it handy).

So I'll withdraw my objection to this change.  I'll dig into your
updated patch momentarily.

jeff

On 08/31/18 18:45, Jeff Law wrote:
> On 08/26/2018 01:45 AM, Bernd Edlinger wrote:
> 
>>>> cp:
>>>> 2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>>>
>>>> 	* decl.c (eval_check_narrowing): Remove.
>>>> 	(check_initializer): Move call to braced_list_to_string from here ...
>>>> 	* typeck2.c (store_init_value): ... to here.
>>>> 	(digest_init_r): Remove handing of signed/unsigned char strings.
>>>>
>>>> testsuite:
>>>> 2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>>>
>>>> 	* c-c++-common/array-init.c: New test.
>>>> 	* g++.dg/init/string2.C: Remove xfail.
>>> My concern here is that you removed code that was explicitly added
>>> during the initial review work of the patch that turned braced
>>> initializers into strings.
>>>
>>
>> Yes, you mean BRACE_ENCLOSED_INITIALIZER_P
>> which is (TREE_CODE (NODE) == CONSTRUCTOR && TREE_TYPE (NODE) == init_list_type_node)
> I was referring to the callback into the eval routine from within
> braced_list_to_string which is related to the concern where you dropped
> the c++98_only selector.  Sorry I wasn't clear about that.
> 

Ah, that you mean.

I think it should be fine, since the main purpose of eval_check_narrowing
was to call check_narrowing on the value, which is normally done by reshape_init
but this is by-passed if braced_list_to_string is successful.

It is much smarter to call braced_list_to_string that after digest_init completed.


> 
>>
>>>
>>>> -    a[] = { 1, 2, 333, 0 };         // { dg-warning "\\\[\(-Wnarrowing|-Woverflow\)" "" { target { ! c++98_only } } }
>>>> +    a[] = { 1, 2, 333, 0 };         // { dg-warning "\\\[\(-Wnarrowing|-Woverflow\)" }
>>> This is not an XFAIL, this is a selector.  Essentially it says that the
>>> diagnostic is appropriate when not in c++98 mode.
>>>
>>> You can see that to be the case if you compile the test in c++98 mode
>>> without your change.  It will compile with no errors or warnings.
>>>
>>> However, after your change it issues a warning for the narrowing
>>> conversion in c++98 mode, which AFAICT it should not do.
>>>
>>
>> This just restores the state _before_ Martin's braced initializer patch.
>> So I have the impression that is actually a regression due to the
>> original braced initializer patch.
>> It is unfortunate that Martin did not check that.
> The ChangeLog said it was removing an xfail, so naturally when I saw it
> was removing a selector I assumed that the selector was right
> (particularly since it made sense given current behavior) and you'd made
> a minor goof.
> 

Yes, indeed, the change log is wrong, it should be

* g++.dg/init/string2.C: Adjust test expectations.


> But I can confirm that prior to Martin's changes we did issue a
> diagnostic when in C++98 mode:
> 
> j.C: In instantiation of ‘int tmplen() [with T = char]’:
> j.C:61:27:   required from here
> j.C:57:5: warning: overflow in conversion from ‘int’ to ‘char’ changes
> value from ‘333’ to ‘'M'’ [-Woverflow]
> 57 |     a[] = { 1, 2, 333, 0 };         // { dg-warning
> "\\\[\(-Wnarrowing|-Woverflow\)" "" { target { ! c++98_only } } }
>     |     ^
> 
> You can see that with the trunk just before Martin's change or with
> older compilers (I also tested with 6.4.1 as I had it handy).
> 
> So I'll withdraw my objection to this change.  I'll dig into your
> updated patch momentarily.
> 
> jeff
> 


Thanks
Bernd.

On 08/31/2018 12:47 AM, Bernd Edlinger wrote:
> On 08/30/18 09:07, Bernd Edlinger wrote:
>> On 08/30/18 06:34, Jason Merrill wrote:
>>> On 08/24/2018 03:52 PM, Bernd Edlinger wrote:
>>>> this updated patch fixes one regression with current trunk due
>>>> to a new test case.  Sorry for the confusion.
>>>>
>>>> The change to the previous version is:
>>>> 1) the check to avoid folding on empty char arrays is restored.
>>>> 2) A null-termination character is added except when the string is full length.
>>>> -              && TYPE_STRING_FLAG (TREE_TYPE (valtype)))
>>>> +      tree typ1 = TYPE_MAIN_VARIANT (TREE_TYPE (type));
>>>> +      if (typ1 == char_type_node
>>>> +      || typ1 == signed_char_type_node
>>>> +      || typ1 == unsigned_char_type_node)
>>>
>>> Why stop using TYPE_STRING_FLAG?
>>>
>> No longer sure, I will try it with TYPE_STRING_FLAG.
>>
> This is an update that uses TYPE_STRING_FLAG.
> It does not seem to make any difference, though.
> 
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> 
> 
> Is it OK for trunk?
> 
> 
> Thanks
> Bernd.
> 
> 
> patch-bracedstr-v2.diff
> 
> 
> c-family:
> 2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* c-common.c (braced_list_to_string): Remove eval parameter.
> 	Add some more checks.  Always create zero-terminated STRING_CST.
> 	* c-common.h (braced_list_to_string): Adjust prototype.
> 
> c:
> 2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* c-decl.c (finish_decl): Call braced_list_to_string here ...
> 	* c-parser.c (c_parser_declaration_or_fndef): ... instead of here.
> 
> 
> cp:
> 2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* decl.c (eval_check_narrowing): Remove.
> 	(check_initializer): Move call to braced_list_to_string from here ...
> 	* typeck2.c (store_init_value): ... to here.
> 	(digest_init_r): Remove handing of signed/unsigned char strings.
> 
> testsuite:
> 2018-08-24  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* c-c++-common/array-init.c: New test.
> 	* g++.dg/init/string2.C: Remove xfail.

Thanks.  Committed.

Jeff