14

This question builds on this @FredOverflow's question.

CLARIFICATION: initializer_list approach is required as the VC++2012 has a bug the prevents forwarded expansion of namespaced arguments. _MSC_VER <= 1700 has the bug.

I've written a variadic template function that collapses any number of arguments in a typed container. I use the type's constructor to convert the variadic arguments into consumable values. E.g. _variant_t :)

I need this for my MySql C++ library when pushing arguments to prepared statements in one strike, while my MySqlVariant converts the input data to MYSQL_BINDs. As I may work with BLOBs, I'd like to avoid copy-construct as much as possible when I can move&& the large containers around.

I've done a simple test and noticed that the initialize_list does copy-construct for the stored elements and destroys them when it goes out of scope. Perfect... Then I tried to move the data out of the initializer_list and, to my surprise, it used lvalues not rvalues as I expected with std::move.

Funny as this happens just after Going Native 2013 clearly warned me that move does not move, forward does not forward... be like water, my friend - to stay on the deep end of thinking.

But that did not stop me :) I decided to const_cast the initializer_list values and still move them out. An eviction order needs to be enforced. And this is my implementation:

template <typename Output_t, typename ...Input_t>
inline Output_t& Compact(Output_t& aOutput, Input_t&& ...aInput){
    // should I do this? makes sense...
    if(!sizeof...(aInput)){
        return aOutput;
    }

    // I like typedefs as they shorten the code :)
    typedef Output_t::value_type Type_t;

    // can be either lvalues or rvalues in the initializer_list when it's populated.
    std::initializer_list<Type_t> vInput = { std::forward<Input_t>(aInput)... };

    // now move the initializer_list into the vector.
    aOutput.reserve(aOutput.size() + vInput.size());
    for(auto vIter(vInput.begin()), vEnd(vInput.end()); vIter != vEnd; ++vIter){
        // move (don't copy) out the lvalue or rvalue out of the initializer_list.
        // aOutput.emplace_back(std::move(const_cast<Type_t&>(*vIter))); // <- BAD!
        // the answer points out that the above is undefined so, use the below
        aOutput.emplace_back(*vIter); // <- THIS is STANDARD LEGAL (copy ctor)!
    }

    // done! :)
    return aOutput;
}

Using it is easy:

// You need to pre-declare the container as you could use a vector or a list...
// as long as .emplace_back is on duty!
std::vector<MySqlVariant> vParams;
Compact(vParams, 1, 1.5, 1.6F, "string", L"wstring",
    std::move(aBlob), aSystemTime); // MySql params :)

I've also uploaded a full test on IDEone ^ that shows as the memory of a std::string moves properly with this function. (I would paste it all here but it's slightly long...)

As long as the _variant_t (or whatever final wrapping object) has the right constructors, it's great. And if the data can be moved out, it's even better. And it pretty much works as I tested it and things std::move in the right direction :)

My questions are simple:

  • Am I doing this right standard-wise?
  • Is the fact that it's working right intended or just a side effect?
  • If std::move does not work by default on initializer_list, is what I'm doing here: illegal, immoral, hacky... or just plain wrong?

PS: I'm a self-taught Windows Native C++ developer, ignorant of the standards.
^ my excuse if I'm doing really non-standard things here.

UPDATE

Thanks everyone, I have both the answer and the solution (a short and long one) now.

And I love the C++11 side of SO. Many knowledgeable people here...

Community
  • 1
  • 1
CodeAngry
  • 12,760
  • 3
  • 50
  • 57
  • 3
    Can't you just do `aOutput.emplace_back(std::forward(aInput))...;` and avoid putting the things in the initializer_list at all? – Billy ONeal Sep 08 '13 at 18:23
  • 1
    Why don't you use array `Type_t vInput[] = ...` instead of `initializer_list`? – catscradle Sep 08 '13 at 18:25
  • @cat: That would result in a copy. – Billy ONeal Sep 08 '13 at 18:26
  • @BillyONeal Doesn't `initializer_list` create an array implicitly anyway? – catscradle Sep 08 '13 at 18:27
  • 2
    @BillyONeal I don't this would work directly, as the pack expansion is illegal in this context IIRC, see [temp.variadic]/4. You could however insert it in a valid context. – dyp Sep 08 '13 at 18:28
  • Of course `std::move` doesn't move. Where would `std::move(some_object)` move the object to? The garbage bin? – fredoverflow Sep 08 '13 at 18:29
  • @BillyONeal My VC++2012 CTP2012 does not like it. Maybe it's a future proof solution but not present proof :) – CodeAngry Sep 08 '13 at 18:32
  • @BillyONeal Is the `...` at the right place? It looks really weird to me, but I'm not used to `...` yet. – fredoverflow Sep 08 '13 at 18:34
  • 2
    @FredOverflow It is at the right place, but in an illegal context for a pack expansion. [Live example](http://coliru.stacked-crooked.com/a/bde55a4f0f1beedf) – dyp Sep 08 '13 at 18:49

3 Answers3

9

In the general case, this is undefined behavior, unfortunately. At §8.5.4/5, emphasis mine:

An object of type std::initializer_list<E> is constructed from an initializer list as if the implementation allocated a temporary array of N elements of type const E, where N is the number of elements in the initializer list. Each element of that array is copy-initialized with the corresponding element of the initializer list, and the std::initializer_list<E> object is constructed to refer to that array.

Where you see a std::initializer_list<E>, you can act as if it's a const E[N].

So when you const_cast away the const, you're looking at a mutable reference to a const object. Any modification to a const object is undefined behavior.

When you move that std::string, you're modifying a const object. Unfortunately , one of the behaviors of undefined behavior is seemingly correct behavior. But this is technically undefined.

Note that when you std::move(int) into another, that is well-defined because int 's can only be copied, so the move does nothing and no const objects are modified. But in general, it's undefined.

GManNickG
  • 494,350
  • 52
  • 494
  • 543
  • Is there any clear reason for this? As the `initialize_list` is designed to be a temporary thing, why can't I take it's belongings?... Is there any scenario where this would crash? (my VC++2012 CTPnov2012 likes it). – CodeAngry Sep 08 '13 at 18:35
  • 3
    @CodeAngry: It's unimportant whether your compiler likes it. The standard says it invokes undefined behaviour. It might for example crash if the `const`-object was placed into read-only memory. – Xeo Sep 08 '13 at 18:40
  • @Xeo **OK! That's a good reason.** *I knew bypassing the standard would get me into trouble.* So... it's back to two implementations. Move one with specialized MySql Variant Param collection and this one for lighter data... I won't mix copy-construct with blobs. – CodeAngry Sep 08 '13 at 18:44
  • @CodeAngry: I cannot comment on the reasoning for making the elements `const`, sorry. – GManNickG Sep 08 '13 at 18:45
  • are there good guarantees about the lifetime? on the max side. – Yakk - Adam Nevraumont Sep 08 '13 at 18:49
  • @Yakk Seems bound to the `initializer_list` scope. As long as you have the list, you have the objects. – CodeAngry Sep 08 '13 at 18:55
  • 2
    @Yakk Just in the next paragraph: "The array has the same lifetime as any other temporary object, except that initializing an `initializer_list` object from the array extends the lifetime of the array exactly like binding a reference to a temporary." – dyp Sep 08 '13 at 18:56
  • @GManNickG There's an interesting note just after the passage you quoted: "[*Note:* The implementation is free to allocate the array in read-only memory if an explicit array with the same initializer could be so allocated. — *end note*]" I suspect this is for e.g. `std::vector v = {1,2,3,4};` where the "expression" on the right side is entirely known at compile-time. – dyp Sep 08 '13 at 19:02
  • @Xeo However. it _is_ possible to move from `const&&`. What OP proposes might be well-defined, even for class types. A `const_cast` would not even be needed. – user1095108 Sep 09 '13 at 11:12
  • @user1095108: Good luck actually moving anything from `const&&` without `const_cast` or `mutable`-hacking-don't-do-it-I-will-slap-you. – Xeo Sep 09 '13 at 11:14
  • @Xeo Just pointing out the view of a language lawyer :) It might be strange, but it is possible. – user1095108 Sep 09 '13 at 11:17
1

Found an alternative solution, for anyone sharing my pain:

#if _MCS_VER <= 1700
// Use the code in the OP!
// VS 2012- stuff comes here.
#else
// VS 2013+ stuff comes here.
template <typename Output_t>
inline Output_t& Compact(Output_t& aOutput){
    return aOutput;
}

template <typename Output_t, typename First_t>
inline Output_t& Compact(Output_t& aOutput, const First_t& aFirst){
    aOutput.emplace_back(aFirst);
    return aOutput;
}

template <typename Output_t, typename First_t>
inline Output_t& Compact(Output_t& aOutput, First_t&& aFirst){
    aOutput.emplace_back(std::move(aFirst));
    return aOutput;
}

template <typename Output_t, typename First_t, typename ...Next_t>
inline Output_t& Compact(Output_t& aOutput, const First_t& aFirst, Next_t&& ...aNext){
    aOutput.emplace_back(aFirst);
    return Compact(aOutput, std::forward<Next_t>(aNext)...);
}

template <typename Output_t, typename First_t, typename ...Next_t>
inline Output_t& Compact(Output_t& aOutput, First_t&& aFirst, Next_t&& ...aNext){
    aOutput.emplace_back(std::move(aFirst));
    return Compact(aOutput, std::forward<Next_t>(aNext)...);
}
#endif // _MCS_VER <= 1700

PS: VC++2012 CTPnov2012 has a BUG that prevents this from working on namespaced classes. So, the initial solution without the const_casthas to do. All my code is namespaced. VC2013 has this fixed in theory... so will switch the code when I upgrade.

CodeAngry
  • 12,760
  • 3
  • 50
  • 57
  • Did you take a look at [catscradle's/my example](http://coliru.stacked-crooked.com/a/fb54acd63349e1ff)? (Does it work on VS2012 Nov CTP?) – dyp Sep 08 '13 at 19:07
  • @DyP Just tried it and it works. If you have a `move&& arg` declared and do a `std::move()` on it in the function body but send a `const ref&` as argument... will it attempt to `const_cast` before trying to `move` or just fall back to a copy construct? *(so I don't go testing it now)* – CodeAngry Sep 08 '13 at 19:14
  • 2
    @DyP Sorry, I posted wrong link earlier, with both old functions. I wanted to do it [like this](http://coliru.stacked-crooked.com/a/2e673bccd067e054). – catscradle Sep 08 '13 at 19:14
  • @DyP It works. Now a stupid question: *why doesn't it work without the wrapping*? – CodeAngry Sep 08 '13 at 19:16
  • 1
    @CodeAngry What wrapping? The reason why `c.emplace_back(args)...;` doesn't "work" is because it's forbidden by the Standard. The pack expansion (expanding the parameter pack `args` via the `...`) is only allowed to appear in certain contexts, probably to clarify the meaning of it. In the context `c.emplace_back(args)...;`, it could be resolved to `c.emplace_back(arg0), c.emplace_back(arg1), /*etc.*/;` but this could lead to side-effects if `,` is overloaded. Maybe there's another rationale why it isn't allowed here. See [this answer](http://stackoverflow.com/a/14430571/420683) – dyp Sep 08 '13 at 19:24
1

You can reduce the specializations by one. This "universal reference" specialization should also cover the lvalue reference, in which case std::move will do nothing.

template <typename Output_t, typename First_t>
inline Output_t& Compact(Output_t& aOutput, First_t&& aFirst){
    aOutput.emplace_back(std::forward<First_t>(aFirst));
    return aOutput;
}

Source: Scott Meyers talk at GoingNative2013; finely detailed in this accu article

Senti Bachcha
  • 78
  • 5
  • 1
    `std::move` is the wrong tool for universal references, `std::forward` is the correct one. – Xeo Sep 09 '13 at 07:29
  • +1. I have modified that in my code `#if _MSC_VER > 1700`. It will work perfectly in VS2013 as right now the bug I mentioned can only be circumvented by an intermediary `initializer_list` for `_MSV_VER <= 1700` :) So I got what I needed working with two versions, fast one that does not work right now... and slower one that'll have to do for the moment. – CodeAngry Sep 09 '13 at 12:06
  • @SentiBachcha The linked PDF was a very good and detailed read. – CodeAngry Sep 09 '13 at 13:49