Re: [boost] proposal a movable_initializer_list

Subject: Re: [boost] proposal a movable_initializer_list
From: 何子杰Hzj_jie (hzj_jie_at_[hidden])
Date: 2014-09-15 22:21:03

sorry, my fault of the mistake of for-range usage, you are correct, I am using iterator in my implementation.


The efficiency I am taking care is the difference between copy-construction and move-construction. It will be significant if we are using lambda expression. Surely we still have something else to improve, say, provide the size of the vector at construction.


The way using movable_initializer_list in the test case is not typical, indeed I am trying another not related issue. <the order of the move-constructions are not expected, i.e. line 46 will throw errors by g++, but no problem by msvc> Usually if a class A accept movable_initializer_list<T> as the parameter of a constructor, we only need A({T(), T()}), i.e. same as initializer_list.


You properly would like to have a look at the event class, which is the typical scenario of movable_initializer_list, at https://github.com/Hzj-jie/osi/blob/master/delegates/event.hpp, with a test at https://github.com/Hzj-jie/osi/blob/master/utt_cases/delegates/event_test.hpp. briefly it uses movable_initializer_list to avoid a copy of std::function instances, but can provide a way to call constructor with { } syntax.





.Hzj_jie





From: Sebastian Redl
Sent: ‎Monday‎, ‎September‎ ‎15‎, ‎2014 ‎8‎:‎09‎ ‎PM
To: boost_at_[hidden]






On 15 Sep 2014, at 9:39, 何子杰Hzj_jie <hzj_jie_at_[hidden]> wrote:

> hi, all,
> 
> So I proposal a movable_initializer_list, it accepts two ways to construct, with an initializer_list<T*> or initializer_list<T>, the prototype is,
> 
> The implementation of T* overloads is something like
> 
> movable_initializer_list<std::initializer_list<T*> x)
> 
> {
> 
>    for(auto it : x)
> 
>    {
> 
>        assert((*it) != nullptr);
> 
>        v.push_back(std::move(**it));
> 
>        delete *it;
> 
>    }
> }

Should be

for (auto p : x) {
  v.push_back(std::move(*p));
  delete p;
}

because of how for-range works. Seems an incomplete manual change from your published source.

However, this is highly problematic because:
- There is absolutely no way to make this exception-safe. The intended usage is obviously:
  SomeType st = { new Foo(123), new Foo(456), new Foo(789) };
  But if any constructor here throws, a lot of memory is leaked, and destructors are not called.
- If efficiency is your goal, I somehow doubt that an additional dynamic allocation per element for the initial list, then building a vector element by element, is the best way to achieve it.
- The interface to this is just horrible. Looking at your test case, there is an absurd amount of boilerplate. Here’s what we really want:
    SomeType st{ Foo(123), Foo(456), Foo(789) };
  Here’s what your thing forces me to do:
    auto pointers = { new Foo(123), new Foo(456), new Foo(789) };
    SomeType st(pointers); // Absolute best case: no ambiguity in overload resolution - actually rather doubtful.
  If you do it that way, you might as well do it this way:
    Foo[] foos = [ Foo(123), Foo(456), Foo(789) }; // No dynamic allocation
    SomeType st(make_move_iterator(begin(foos)), make_move_iterator(end(foos)));

More efficient, and you could make helpers that combine the make_move_iterator and begin/end calls to reduce boilerplate.

> 
> While the implementation of T overloads is something like
> 
> moveable_initializer_list<std::initializer_list<T> x)
> {
> 
>    for(auto it : x) v.push_back(move(*it));
> }

Again, that’s not how for-range works. What you want is

for (auto&& e : x) v.push_back(std::move(e));

But that doesn’t actually work, because a std::initializer_list never gives you non-const access to its elements.

I notice that your test case doesn’t test this constructor.

Sebastian


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost