On 2/21/20 7:41 PM, Martin Sebor wrote:
On 2/17/20 10:54 AM, Jason Merrill wrote:
On 2/14/20 9:06 PM, Martin Sebor wrote:
On 2/13/20 3:59 PM, Jason Merrill wrote:
On 2/12/20 9:21 PM, Martin Sebor wrote:
On 2/11/20 5:28 PM, Jason Merrill wrote:
On 2/11/20 9:00 PM, Martin Sebor wrote:
r270155, committed in GCC 9, introduced a transformation that strips
redundant trailing zero initializers from array initializer lists in
order to support string literals as template arguments.
The transformation neglected to consider the case of array elements
of trivial class types with user-defined conversion ctors and either
defaulted or deleted default ctors. (It didn't occur to me that
those qualify as trivial types despite the user-defined ctors.) As
a result, some valid initialization expressions are rejected when
the explicit zero-initializers are dropped in favor of the (deleted)
default ctor,
Hmm, a type with only a deleted default constructor is not
trivial, that should have been OK already.
For Marek's test case:
struct A { A () == delete; A (int) = delete; };
trivial_type_p() returns true (as does __is_trivial (A) in both GCC
and Clang).
[class.prop] says that
A trivial class is a class that is trivially copyable and has one
or more default constructors (10.3.4.1), all of which are either
trivial or deleted and at least one of which is not deleted.
That sounds like A above is not trivial because it doesn't have
at least one default ctor that's not deleted, but both GCC and
Clang say it is. What am I missing? Is there some other default
constructor hiding in there that I don't know about?
and others are eliminated in favor of the defaulted
ctor instead of invoking a user-defined conversion ctor, leading to
wrong code.
This seems like a bug in type_initializer_zero_p; it shouldn't
treat 0 as a zero initializer for any class.
That does fix it, and it seems like the right solution to me as well.
Thanks for the suggestion. I'm a little unsure about the condition
I put in place though.
Attached is an updated patch rested on x86_64-linux.
- if (sized_array_p && trivial_type_p (elt_type))
+ if (sized_array_p
+ && trivial_type_p (elt_type)
+ && !TYPE_NEEDS_CONSTRUCTING (elt_type))
Do we still need this change? If so, please add a comment about the
trivial_type_p bug.
The change isn't needed with my patch as it was, but it would still
be needed with the changes you suggested (even then it doesn't help
with the problem I describe below).
if (TREE_CODE (init) != CONSTRUCTOR
I might change this to
if (!CP_AGGREGATE_TYPE_P (type))
return initializer_zerop (init);
This behaves differently in C++ 2a mode (the whole condition evaluates
to true for class A below) than in earlier modes and causes a failure
in the new array55.C test:
True, my suggestion above does the wrong thing for non-aggregate classes.
+ /* A class can only be initialized by a non-class type if it has
+ a ctor that converts from that type. Such classes are excluded
+ since their semantics are unknown. */
+ if (RECORD_OR_UNION_TYPE_P (type)
+ && !RECORD_OR_UNION_TYPE_P (TREE_TYPE (init)))
+ return false;
How about if (!SCALAR_TYPE_P (type)) here?
More broadly, it seems like doing this optimization here at all is
questionable, since we don't yet know whether there's a valid
conversion from the zero-valued initializer to the element type. It
would seem better to do it in process_init_constructor_array after the
call to massage_init_elt, when we know the actual value of the element.
Yes, it seems that it might be better to do it there. Attached
is a revised patch that implements your suggestion. It's probably
more intrusive than you envisioned. The stripping of the redundant
trailing initializers was straightforward. Most of the rest of
the changes are only necessary to strip redundant initializers
for pointers to data members.
Martin
PS I'm uneasy about this patch this late in the cycle. The bug I'm
fixing was introduced at the end of the last release, as a result
of a last minute patch not unlike this one. It caused at least two
codegen regressions in all language modes. I'd hate for this change
to have similar repercussions.
Hmm, yes, the last_distinct logic is a bit opaque. Please rework it so
you aren't changing i within the loop.
Jason
