Fix storage move constructor

[toffaletti]

Stop compiler from thinking we're trying to use an initializer list

Repro:

#include <core/optional.hpp>

struct A {};

core::optional<A> foo() {
    return core::nullopt;
}

int main() {                                                                                                                                                                                                                                                                                                                                                                
    const auto a = foo();
}

g++ -std=c++11 const.cc 
In file included from const.cc:1:0:
/usr/include/core/optional.hpp: In instantiation of ‘core::v1::impl::storage<T, true>::storage(core::v1::impl::storage<T, true>&&) [with T = A]’:
/usr/include/core/optional.hpp:193:3:   required from here
/usr/include/core/optional.hpp:108:5: error: could not convert ‘{core::v1::move<A&>((* & that.core::v1::impl::storage<A, true>::<anonymous>.core::v1::impl::storage<A, true>::<anonymous union>::val))}’ from ‘<brace-enclosed initializer list>’ to ‘core::v1::impl::storage<A, true>::value_type {aka A}’
     ::new(::std::addressof(this->val)) value_type {
     ^

clang -std=c++11 const.cc 
In file included from const.cc:1:
/usr/include/core/optional.hpp:109:7: error: excess elements in struct initializer
      ::core::move(that.val)
      ^~~~~~~~~~~~~~~~~~~~~~
/usr/include/core/optional.hpp:193:3: note: in instantiation of member function 'core::v1::impl::storage<A, true>::storage' requested here
  optional (optional&&) = default;
  ^
1 error generated.

I find it really annoying that neither g++ or clang references the line of code in my example that triggers this error.

[toffaletti]

This is a problem in several other places in the code, like line 62 and anywhere else move is used with {} initializer syntax. I don't expect you to merge this, it is more of a bug report and suggestion.

[bruxisma]

OK, so I did some digging into this because I was fairly certain that the language permits the uniform initialization with placement new (it does), and I found that both compilers not reporting the correct line where your code breaks to be extremely bizarre. They're both usually pretty good about saying where the line of code that instantiated the issue occurred. I found a bug for clang that is still open but is apparently being tracked internally at Apple, so I'm unable to go any further down that route.

However, because the bug mentions

Using placement new fails, when using uniform initialization without explicitly defined constructor.

I placed a defaulted move and copy constructor in the empty struct (but not a defaulted default constructor), and received a different error from gcc (4.8.1 mingw64):

include/core/optional.hpp:108:5: error: too many initializers for 'main()::__lambda29::A'
 ::new (::std::addressof(this->val)) value_type { ::core::move(that.val) };
 ^

If I add in a defaulted default constructor, I get the same error you got before.

Here's where things get interesting, because this might be a compiler bug, or several compiler bugs. If I add a single member to A, so that its definition becomes:

struct A {
  int x { 0 };
};

It compiles correctly. Additionally, if I change the member to be a single char (which would make the struct have the same size as if it were empty, but remove the chance for empty base optimization), it also still compiles. However if the default initialization of the member is removed, you get the same error as before.

I'm wondering if the compilers always attempt empty base optimization (otherwise the size of objects would most likely differ between debug and release builds, and break in terrible ways) and this is causing some weird state internally. I've no way to prove this of course, and its just a theory. It's most likely shot down by the default initialization of the member changing how everything is affected.

I apologize, but the machine that had clang installed has died on me in the past month and I am unable to afford replacing or repairing it.

Now I assume the reason that Clang's <experimental/optional> does not run into this problem is that they use the parentheses way of performing placement new (which is what your pull request does).

Aside from your pull request, I was wondering if you ran into this issue with a non-empty class?

I'll be fixing this on the whole to use parenthetical initialization with placement new in the entire class (and I also still have yet to finish the contributor license agreement before I can accept pull requests)

Also, apologies for not responding to this as timely as I have with everything else.

[toffaletti]

I did run into it with a non-empty class. I'll take a look at the clang radar and maybe get better insight into what is going on.

[toffaletti]

That clang bug is for something different, a crash that was happening because of a misplaced assert().

[toffaletti]

It looks like you missed some. I'm still getting an error here:

g++ -std=c++11 move.cc 
In file included from move.cc:1:0:
/usr/include/core/optional.hpp: In instantiation of ‘constexpr core::v1::impl::storage<T, true>::storage(core::v1::impl::storage<T, true>::value_type&&) [with T = A; core::v1::impl::storage<T, true>::value_type = A]’:
/usr/include/core/optional.hpp:201:32:   required from ‘constexpr core::v1::optional<Type>::optional(core::v1::optional<Type>::value_type&&) [with Type = A; core::v1::optional<Type>::value_type = A]’
move.cc:8:14:   required from here
/usr/include/core/optional.hpp:118:20: error: too many initializers for ‘core::v1::impl::storage<A, true>::value_type {aka A}’
     engaged { true }
                    ^

This at least gives me a line number in my code, but it gets the line number wrong in optional.hpp. The error is actually on the previous line and its the same pattern: move inside initializer braces. Line 72 has a similar issue, but I'm having trouble writing a small test case to reproduce it.

#include <core/optional.hpp>

using namespace core;

struct A {};

optional<A> foo() {
    return A{};
}

int main() {
    const auto a = foo();
}

[bruxisma]

This has to be the weirdest bug I've encountered in a while (and I ran into some really wonky stuff with core::variant<Ts...>).

I'm going to have to go through and fix this for core::variant<Ts...> as well, since it is affected in the same way. I'll have to make it a personal note that any placement new using move semantics can't use brace initialization, or this could happen again.

Thanks for finding this out, by the way. It's a pretty big one.

[bruxisma]

OK. So I have an update on this issue, including why brace initialization breaks.

Turns out because the struct (and I assume the other types you've run into) is an aggregate, you can't actually use brace initialization. Aggregates can only be initialized with an empty list (this is why the core::result<T> didn't have this issue beyond its move constructor, because all placement new used an empty initialization list. This also explains the weird errors we were getting. Because I had declared a default move and copy, it no longer made the struct an aggregate and therefore resulted in it compiling correctly.

This also of course affects the core::variant<Ts...> type, as well as core::expected<T> and core::result<T>, but only in certain parts.

So much for my uniform code, I've not been a fan of parenthetical initialization, but oh well.

[toffaletti]

That makes sense, I know I've run into this issue before but I didn't bother to understand why, I just fixed the code and moved on. I got all excited by c++11 promise of "uniform initialization syntax" only to run into edge cases like this or containers like std::vector where it changes the meaning of the code and causes hard to find runtime bugs. What a let down.