On Fri, Jan 17, 2025 at 07:00:33PM -0500, Jason Merrill wrote:
> > --- gcc/c-family/c-common.cc.jj 2025-01-02 11:47:29.803228077 +0100
> > +++ gcc/c-family/c-common.cc 2025-01-17 13:32:53.512294482 +0100
> > @@ -9016,9 +9016,26 @@ vec<tree, va_gc> *
> > make_tree_vector_from_ctor (tree ctor)
> > {
> > vec<tree,va_gc> *ret = make_tree_vector ();
> > + unsigned nelts = CONSTRUCTOR_NELTS (ctor);
> > vec_safe_reserve (ret, CONSTRUCTOR_NELTS (ctor));
> > for (unsigned i = 0; i < CONSTRUCTOR_NELTS (ctor); ++i)
> > - ret->quick_push (CONSTRUCTOR_ELT (ctor, i)->value);
> > + if (TREE_CODE (CONSTRUCTOR_ELT (ctor, i)->value) == RAW_DATA_CST)
> > + {
> > + tree raw_data = CONSTRUCTOR_ELT (ctor, i)->value;
> > + nelts += RAW_DATA_LENGTH (raw_data);
> > + vec_safe_reserve (ret, nelts);
>
> I notice that the second argument to vec_safe_reserve is the number of empty
> spaces to provide, rather than the new desired capacity. So this will
> allocate space for an additional copy of the whole constructor.
You're right.
> I guess you want to say reserve (ret, nelts - vec_safe_length (res))? It's
> weird that there isn't a more convenient way to express that.
I think vec_safe_reserve (ret, nelts - ret->length ()); is enough, the first
vec_safe_reserve already necessarily make ret non-NULL because if it would
not have reserved anything (nelts == 0), then there would be no ctor
elements to walk.
> Note that this behavior differs from the C++ standard library, where reserve
> indeed specifies the new capacity. This seems likely to cause a lot of
> confusion (and over-allocation) going forward. And indeed looking through
> exising uses turns up at least two that seem to assume the standard library
> semantics (as well as others that assume the actual semantics).
Which ones did you find?
To me cp/name-lookup,cc (name_lookup::preserve_state)
unsigned length = vec_safe_length (previous->scopes);
vec_safe_reserve (previous->scopes, length * 2);
looks possibly unintended, as it quick_pushes at most length further
elements.
I've so far skimmed just vec_safe_reserve uses in {c-family,c,cp}
subdirectories, there are 26 further vec_safe_reserve and around 500
.reserve or ->reserve cases. Most of the vec_safe_reserve and reserve
calls are on empty vectors so it really doesn't matter which one is which.
So, I wonder if at least during the conversion it wouldn't be best to have
all 3 namings, reserve, reserve_space and reserve_capacity, where the first
one would at runtime assert that length () is 0.
> I wonder about renaming the existing reserve to reserve_space and adding a
> reserve_capacity?
Would it be ok to first handle these PRs (fixed patch below) and go through
the renaming and checking incrementally?
There was another thinko in the patch, nelts += RAW_DATA_LENGTH (raw_data);
would mean again reserving too much, it needs to be nelts += RAW_DATA_LENGTH
(raw_data) - 1, as we don't push the RAW_DATA_CST itself which was already
accounted for, but replace it with RAW_DATA_LENGTH pushes.
So far tested on
GXX_TESTSUITE_STDS=98,11,14,17,20,23,26 make check-g++
RUNTESTFLAGS="dg.exp='embed* class-deduction-aggr*.C explicit*.C pr11853[24]*'"
ok if it passes full bootstrap+regtest?
2025-01-18 Jakub Jelinek <ja...@redhat.com>
PR c++/118528
* c-common.cc (make_tree_vector_from_ctor): Expand RAW_DATA_CST
elements from the CONSTRUCTOR to individual INTEGER_CSTs.
* g++.dg/cpp/embed-21.C: New test.
* g++.dg/cpp2a/class-deduction-aggr16.C: New test.
--- gcc/c-family/c-common.cc.jj 2025-01-02 11:47:29.803228077 +0100
+++ gcc/c-family/c-common.cc 2025-01-17 13:32:53.512294482 +0100
@@ -9016,9 +9016,26 @@ vec<tree, va_gc> *
make_tree_vector_from_ctor (tree ctor)
{
vec<tree,va_gc> *ret = make_tree_vector ();
+ unsigned nelts = CONSTRUCTOR_NELTS (ctor);
vec_safe_reserve (ret, CONSTRUCTOR_NELTS (ctor));
for (unsigned i = 0; i < CONSTRUCTOR_NELTS (ctor); ++i)
- ret->quick_push (CONSTRUCTOR_ELT (ctor, i)->value);
+ if (TREE_CODE (CONSTRUCTOR_ELT (ctor, i)->value) == RAW_DATA_CST)
+ {
+ tree raw_data = CONSTRUCTOR_ELT (ctor, i)->value;
+ nelts += RAW_DATA_LENGTH (raw_data) - 1;
+ vec_safe_reserve (ret, nelts - ret->length ());
+ if (TYPE_PRECISION (TREE_TYPE (raw_data)) > CHAR_BIT
+ || TYPE_UNSIGNED (TREE_TYPE (raw_data)))
+ for (unsigned j = 0; j < (unsigned) RAW_DATA_LENGTH (raw_data); ++j)
+ ret->quick_push (build_int_cst (TREE_TYPE (raw_data),
+ RAW_DATA_UCHAR_ELT (raw_data, j)));
+ else
+ for (unsigned j = 0; j < (unsigned) RAW_DATA_LENGTH (raw_data); ++j)
+ ret->quick_push (build_int_cst (TREE_TYPE (raw_data),
+ RAW_DATA_SCHAR_ELT (raw_data, j)));
+ }
+ else
+ ret->quick_push (CONSTRUCTOR_ELT (ctor, i)->value);
return ret;
}
--- gcc/testsuite/g++.dg/cpp/embed-21.C.jj 2025-01-17 12:18:00.571912195
+0100
+++ gcc/testsuite/g++.dg/cpp/embed-21.C 2025-01-17 13:37:31.478489868 +0100
@@ -0,0 +1,22 @@
+// PR c++/118528
+// { dg-do compile { target c++20 } }
+// { dg-options "" }
+
+template<class T>
+struct E { T t[130][2]; };
+
+E e1 {
+#embed __FILE__ limit (260)
+};
+
+template<class T>
+struct F { T t[2][130]; };
+
+F f1 {
+#embed __FILE__ limit (260)
+};
+F f2 { { {
+#embed __FILE__ limit (130)
+}, {
+#embed __FILE__ limit (130)
+} } };
--- gcc/testsuite/g++.dg/cpp2a/class-deduction-aggr16.C.jj 2025-01-17
12:17:23.715424611 +0100
+++ gcc/testsuite/g++.dg/cpp2a/class-deduction-aggr16.C 2025-01-17
12:17:43.717146527 +0100
@@ -0,0 +1,17 @@
+// PR c++/118528
+// { dg-do compile { target c++20 } }
+
+template<class T>
+struct E { T t[130][2]; };
+
+#define P 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16
+#define Q { 1, 2 }, { 3, 4 }, { 5, 6 }, { 7, 8 }, { 9, 10 }, { 11, 12 }, \
+ { 13, 14 }, { 15, 16 }
+E e1 { P, P, P, P, P, P, P, P, P, P, P, P, P, P, P, P, 1, 2, 3, 4 };
+E e2 { { Q, Q, Q, Q, Q, Q, Q, Q, Q, Q, Q, Q, Q, Q, Q, Q, { 1, 2 }, { 3, 4 } }
};
+
+template<class T>
+struct F { T t[2][130]; };
+
+F f1 { P, P, P, P, P, P, P, P, P, P, P, P, P, P, P, P, 1, 2, 3, 4 };
+F f2 { { { P, P, P, P, P, P, P, P, 1, 2 }, { P, P, P, P, P, P, P, P, 3, 4 } }
};
Jakub
