Re: [C++ PATCH] preview: Fix braces around scalar initializer (C++/88572) Inbox x
The patch looks good, but still needs a ChangeLog entry. And please
send patches to the mailing list even if you're also attaching them to
the PR.
It also looks like you don't have a copyright assignment on file yet;
this patch is small enough to go in without it, but you probably want
to get that out of the way to simplify future patch submissions.
Thanks,
Jason
On Wed, Feb 20, 2019 at 7:58 AM will wray wrote:
>
> I've updated the patch to address both comments; the first conditional
> now deals only with C++98, an extra 'else if' block deals with both
> empty scalar var init and scalar sub-object init with too many braces.
>
> I'll bump from 'preview' to 'proposed' -
> please review for inclusion on trunk and possible backports.
>
> I've stressed the patch with my own code which does SFINAE on these errors;
> all is working as expected, portable with Clang trunk.
>
> It bootstraps and regtests for me on x86_64-linux.
>
> On Fri, Feb 15, 2019 at 11:15 PM Jason Merrill wrote:
>>
>> On 2/14/19 7:09 PM, will wray wrote:
>> > Thanks Jason.
>> > Adding this 'else if' condition afterwards seems to work:
>> >
>> > else if (BRACE_ENCLOSED_INITIALIZER_P (CONSTRUCTOR_ELT
>> > (stripped_init,0)->value))
>> > {
>> >if (complain & tf_error)
>> > error ("too many braces around scalar initializer for
>> > type %qT", type);
>> >init = error_mark_node;
>> > }
>> >
>> > I'll regtest that and run through the rest of the reshape logic again.
>>
>> I think the first_initializer_p check should be part of this condition
>> rather than the C++98 condition.
>> > What do you think about the fact that this patch now rejects empty
>> > brace inits like int{{}}
>> > that was previously accepted? It's a breaking change for any code that
>> > was incorrectly doing that.
>>
>> The change makes sense to me; I would hope that such code is rare.
>>
>> Jason
>>
>> > On Thu, Feb 14, 2019 at 6:02 PM Jason Merrill wrote:
>> >>
>> >> On 2/12/19 6:04 PM, will wray wrote:
>> >>> A proposed patch for Bug 88572 is attached to the bug report along
>> >>> with a short description and Change Log (a link there gives a pretty
>> >>> diff of the patch):
>> >>>
>> >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88572#c15
>> >>>
>> >>> I'd appreciate any review of this patch, as well as testing on more
>> >>> platforms. The patch with updated tests passes for me on x86_64.
>> >>>
>> >>> There's also test code in bug comment #1 that demonstrates SFINAE
>> >>> based on the nesting of braces. It could also be added to the
>> >>> testsuite - I'm not sure how to do that or if it is needed.
>> >>
>> >>> + if (cxx_dialect < cxx11 || first_initializer_p)
>> >>
>> >> I would expect this to miss the error in
>> >>
>> >> struct A { int i; } a = {{{42}}};
>> >>
>> >> I see that we end up complaining about this in convert_like_real because
>> >> implicit_conversion catches the problem here, but I think we ought to
>> >> catch it in reshape_init_r as well. So, also complain if the element of
>> >> the CONSTRUCTOR is also BRACE_ENCLOSED_INITIALIZER_P.
>> >>
>> >> Jason
>>
Re: [C++ PATCH] preview: Fix braces around scalar initializer (C++/88572) Inbox x
I've updated the patch to address both comments; the first conditional
now deals only with C++98, an extra 'else if' block deals with both
empty scalar var init and scalar sub-object init with too many braces.
I'll bump from 'preview' to 'proposed' -
please review for inclusion on trunk and possible backports.
I've stressed the patch with my own code which does SFINAE on these errors;
all is working as expected, portable with Clang trunk.
It bootstraps and regtests for me on x86_64-linux.
On Fri, Feb 15, 2019 at 11:15 PM Jason Merrill wrote:
> On 2/14/19 7:09 PM, will wray wrote:
> > Thanks Jason.
> > Adding this 'else if' condition afterwards seems to work:
> >
> > else if (BRACE_ENCLOSED_INITIALIZER_P (CONSTRUCTOR_ELT
> > (stripped_init,0)->value))
> > {
> >if (complain & tf_error)
> > error ("too many braces around scalar initializer for
> > type %qT", type);
> >init = error_mark_node;
> > }
> >
> > I'll regtest that and run through the rest of the reshape logic again.
>
> I think the first_initializer_p check should be part of this condition
> rather than the C++98 condition.
> > What do you think about the fact that this patch now rejects empty
> > brace inits like int{{}}
> > that was previously accepted? It's a breaking change for any code that
> > was incorrectly doing that.
>
> The change makes sense to me; I would hope that such code is rare.
>
> Jason
>
> > On Thu, Feb 14, 2019 at 6:02 PM Jason Merrill wrote:
> >>
> >> On 2/12/19 6:04 PM, will wray wrote:
> >>> A proposed patch for Bug 88572 is attached to the bug report along
> >>> with a short description and Change Log (a link there gives a pretty
> >>> diff of the patch):
> >>>
> >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88572#c15
> >>>
> >>> I'd appreciate any review of this patch, as well as testing on more
> >>> platforms. The patch with updated tests passes for me on x86_64.
> >>>
> >>> There's also test code in bug comment #1 that demonstrates SFINAE
> >>> based on the nesting of braces. It could also be added to the
> >>> testsuite - I'm not sure how to do that or if it is needed.
> >>
> >>> + if (cxx_dialect < cxx11 || first_initializer_p)
> >>
> >> I would expect this to miss the error in
> >>
> >> struct A { int i; } a = {{{42}}};
> >>
> >> I see that we end up complaining about this in convert_like_real because
> >> implicit_conversion catches the problem here, but I think we ought to
> >> catch it in reshape_init_r as well. So, also complain if the element of
> >> the CONSTRUCTOR is also BRACE_ENCLOSED_INITIALIZER_P.
> >>
> >> Jason
>
>
Re: [C++ PATCH] preview: Fix braces around scalar initializer (C++/88572) Inbox x
On 2/14/19 7:09 PM, will wray wrote:
Thanks Jason.
Adding this 'else if' condition afterwards seems to work:
else if (BRACE_ENCLOSED_INITIALIZER_P (CONSTRUCTOR_ELT
(stripped_init,0)->value))
{
if (complain & tf_error)
error ("too many braces around scalar initializer for
type %qT", type);
init = error_mark_node;
}
I'll regtest that and run through the rest of the reshape logic again.
I think the first_initializer_p check should be part of this condition
rather than the C++98 condition.
What do you think about the fact that this patch now rejects empty
brace inits like int{{}}
that was previously accepted? It's a breaking change for any code that
was incorrectly doing that.
The change makes sense to me; I would hope that such code is rare.
Jason
On Thu, Feb 14, 2019 at 6:02 PM Jason Merrill wrote:
On 2/12/19 6:04 PM, will wray wrote:
A proposed patch for Bug 88572 is attached to the bug report along
with a short description and Change Log (a link there gives a pretty
diff of the patch):
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88572#c15
I'd appreciate any review of this patch, as well as testing on more
platforms. The patch with updated tests passes for me on x86_64.
There's also test code in bug comment #1 that demonstrates SFINAE
based on the nesting of braces. It could also be added to the
testsuite - I'm not sure how to do that or if it is needed.
+ if (cxx_dialect < cxx11 || first_initializer_p)
I would expect this to miss the error in
struct A { int i; } a = {{{42}}};
I see that we end up complaining about this in convert_like_real because
implicit_conversion catches the problem here, but I think we ought to
catch it in reshape_init_r as well. So, also complain if the element of
the CONSTRUCTOR is also BRACE_ENCLOSED_INITIALIZER_P.
Jason
Re: [C++ PATCH] preview: Fix braces around scalar initializer (C++/88572) Inbox x
On 2/12/19 6:04 PM, will wray wrote:
A proposed patch for Bug 88572 is attached to the bug report along
with a short description and Change Log (a link there gives a pretty
diff of the patch):
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88572#c15
I'd appreciate any review of this patch, as well as testing on more
platforms. The patch with updated tests passes for me on x86_64.
There's also test code in bug comment #1 that demonstrates SFINAE
based on the nesting of braces. It could also be added to the
testsuite - I'm not sure how to do that or if it is needed.
+ if (cxx_dialect < cxx11 || first_initializer_p)
I would expect this to miss the error in
struct A { int i; } a = {{{42}}};
I see that we end up complaining about this in convert_like_real because
implicit_conversion catches the problem here, but I think we ought to
catch it in reshape_init_r as well. So, also complain if the element of
the CONSTRUCTOR is also BRACE_ENCLOSED_INITIALIZER_P.
Jason
[C++ PATCH] preview: Fix braces around scalar initializer (C++/88572) Inbox x
A proposed patch for Bug 88572 is attached to the bug report along with a short description and Change Log (a link there gives a pretty diff of the patch): https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88572#c15 I'd appreciate any review of this patch, as well as testing on more platforms. The patch with updated tests passes for me on x86_64. There's also test code in bug comment #1 that demonstrates SFINAE based on the nesting of braces. It could also be added to the testsuite - I'm not sure how to do that or if it is needed. braces_patch Description: Binary data
