std:vec for classes with constructor?

[Richard Biener]

On August 10, 2020 3:51:28 PM GMT+02:00, Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>On Mon, Aug 10, 2020 at 02:57:31PM +0200, Aldy Hernandez wrote:
>> > I think it would be nice to see e.g. in -fdump-tree-gimple dump
>> > on ipa-fnsummary.c what value_range ctors does it call for the
>auto_vec and
>> > if that is what we want, other than that LGTM.
>> 
>> I see the following in the dump:
>> 
>> evaluate_properties_for_edge()
>> {
>> ...
>>   struct auto_vec known_value_ranges;
>> ...
>>   try
>>     {
>> ...
>>       auto_vec<int_range<1>, 32>::auto_vec (&known_value_ranges);
>> ...
>> 
>> It looks like the auto_vec constructor is calling the int_range<1>
>> constructor correctly:
>> 
>> auto_vec<int_range<1>, 32>::auto_vec (struct auto_vec * const this)
>> {
>> ...
>>     int_range<1>::int_range (D.130911);
>> 
>> 
>> The int_range<1> constructor also looks correct:
>> 
>> int_range<1>::int_range (struct int_range * const this, const struct
>> int_range & other)
>> {
>>   *this = {CLOBBER};
>>   {
>>     _1 = &this->D.92184;
>>     _2 = &this->m_ranges;
>>     irange::irange (_1, _2, 1);
>>     _3 = &this->D.92184;
>>     _4 = &other->D.92184;
>>     irange::operator= (_3, _4);
>>   }
>> }
>> 
>> If y'all agree, I will push the patch with Jakub's suggested changes.
>
>I believe even with that change vec doesn't work properly with
>(arbitrary)
>non-PODs that need non-trivial construction or destruction.
>I know you haven't introduced that yourself, but just would like to
>discuss
>the semantics we want.
>
>To test, I've tried:
>--- vec.c.xx	2020-01-12 11:54:38.533381424 +0100
>+++ vec.c	2020-08-10 15:33:40.600680476 +0200
>@@ -544,9 +544,34 @@ test_auto_delete_vec ()
> 
> /* Run all of the selftests within this file.  */
> 
>+struct SSXS
>+{
>+  SSXS () { __builtin_printf ("SSXS ()\n"); }
>+  SSXS (const SSXS &) { __builtin_printf ("SSXS (const SSXS &)\n"); }
>+  SSXS &operator= (const SSXS &) { __builtin_printf ("operator= (const
>SSXS &)\n"); }
>+  ~SSXS () { __builtin_printf ("~SSXS ()\n"); }
>+  int s;
>+};
>+
> void
> vec_c_tests ()
> {
>+  {
>+    auto_vec<SSXS, 8> ssxs;
>+    __builtin_printf ("constructed\n");
>+    SSXS ssxso;
>+    __builtin_printf ("ssxso\n");
>+    for (int i = 0; i < 16; i++)
>+      {
>+        __builtin_printf ("safe_push start\n");
>+        {
>+          ssxs.safe_push (ssxso);
>+        }
>+        __builtin_printf ("safe_push end\n");
>+      }
>+    __builtin_printf ("about to destruct\n");
>+  }
>+  __builtin_printf ("destructed\n");
>   test_quick_push ();
>   test_safe_push ();
>   test_truncate ();
>
>and I see following.  Let me add comments to that.
>
>#So auto_vec constructor will default initialize all m_data
>#elements (i.e. all the embedded objects):
>SSXS ()
>SSXS ()
>SSXS ()
>SSXS ()
>SSXS ()
>SSXS ()
>SSXS ()
>SSXS ()
>constructed
>#And now the extra object I've created.
>SSXS ()
>ssxso
># Now, 8 times we invoke an assignment operator, the objects
># are already constructed, so all is good.
>safe_push start
>operator= (const SSXS &)
>safe_push end
>safe_push start
>operator= (const SSXS &)
>safe_push end
>safe_push start
>operator= (const SSXS &)
>safe_push end
>safe_push start
>operator= (const SSXS &)
>safe_push end
>safe_push start
>operator= (const SSXS &)
>safe_push end
>safe_push start
>operator= (const SSXS &)
>safe_push end
>safe_push start
>operator= (const SSXS &)
>safe_push end
>safe_push start
>operator= (const SSXS &)
>safe_push end
># Now, 9th safe_push, we ran out of room in the embedded buffer.
># vec_copy_construct will now copy construct the 8 elements
># in the new array which will be used from now on.
>safe_push start
>SSXS (const SSXS &)
>SSXS (const SSXS &)
>SSXS (const SSXS &)
>SSXS (const SSXS &)
>SSXS (const SSXS &)
>SSXS (const SSXS &)
>SSXS (const SSXS &)
>SSXS (const SSXS &)
># But important note, we do not construct the object at new
>m_vecdata[8],
># only call assignment operator on it.  That is invalid C++, isn't it?
>operator= (const SSXS &)
>safe_push end
>safe_push start
># And again, and again ...
>operator= (const SSXS &)
>safe_push end
>safe_push start
>operator= (const SSXS &)
>safe_push end
>safe_push start
>operator= (const SSXS &)
>safe_push end
>safe_push start
>operator= (const SSXS &)
>safe_push end
>safe_push start
>operator= (const SSXS &)
>safe_push end
>safe_push start
>operator= (const SSXS &)
>safe_push end
>safe_push start
>operator= (const SSXS &)
>safe_push end
>about to destruct
># Now we destruct the 8 embedded elts in auto_vec plus the variable
># I had there, but the 16 elts of the m_vecdata array, 8 of those
># were copy constructed and then operator= modified, while
># the other 8 were only operator= set, are not destructed ever.
>~SSXS ()
>~SSXS ()
>~SSXS ()
>~SSXS ()
>~SSXS ()
>~SSXS ()
>~SSXS ()
>~SSXS ()
>~SSXS ()
>destructed
>
>So, for vec on types with non-trivial construction and/or destruction,
>the
>question is, do we want to construct the elements whenever underlying
>storage for those is allocated (i.e. as now let the embedded buffer
>have
>constructed elements and whenever we allocate new memory for the
>vector,
>next to the vec_copy_construct where we copy construct the oldsize
>elts,
>we also default construct the remaining ones) and then destruct when
>deallocating the memory and then just use assignment operator to push
>stuff
>in, or do we want the memory uninitialized instead and only copy
>construct
>during quick_push and destruct during pop etc., i.e. only the currently
>active elts are constructed and nothing else?  E.g. auto_vec m_data
>would
>then need to be a different type (e.g. char array with appropriate
>alignas
>and size) which wouldn't need non-trivial construction and destruction.
>
>For types with trivial construction/destruction, I'd like to keep
>current
>behavior...

We want to construct / destruct on push/pop, not on allocation. We also want to use std::move upon reallocation (but then we can't use realloc, can we?) 

Richard. 

>	Jakub