[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gcc-patches
Subject:    Re: [PATCHv2] Call braced_list_to_string after array size is fixed
From:       Bernd Edlinger <bernd.edlinger () hotmail ! de>
Date:       2018-08-31 17:38:24
Message-ID: VI1PR0701MB28627FF91F5336C1BA12B00EE40F0 () VI1PR0701MB2862 ! eurprd07 ! prod ! outlook ! com
[Download RAW message or body]

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.


[prev in list] [next in list] [prev in thread] [next in thread]