Fix bugs in in ctl::optional

[alkis]

Manually manage the lifetime of value_ by using an anonymous
union. This fixes a bunch of double-frees and double-constructs.

Additionally move the present_ flag last. When T has padding
present_ will be placed there saving alignof(T) bytes from
sizeof(optional<T>).

[mrdomino]

LGTM

I like the approach. I'm assuming that you've tested and verified that this does indeed behave the way you expect it to.

I'm trying to think about why I've seen other people do alignas(T) char blob[sizeof(T)] instead of a union; but I can't come up with a reason to do one vs the other right now.

As far as I can tell, manually managed lifetimes is pretty much table stakes for writing an optional.

I believe there may be some CLA type actions that @jart can help you with if this is your first contribution to cosmopolitan.

[alkis]

I didn't test this in this PR but I have implemented too many "optionals" I would want to admit.

I sent @jart the CLA already.

[alkis]

alignas(T) char blob[sizeof(T)]

This was done before C++11 because unions could not take non-POD types before that.

From https://en.cppreference.com/w/cpp/language/union. Until C++11:

Unions cannot contain a non-static data member with a non-trivial special member function (copy constructor, copy-assignment operator, or destructor).

C+11 onwards:

If a union contains a non-static data member with a non-trivial special member function (copy/move constructor, copy/move assignment, or destructor), that function is deleted by default in the union and needs to be defined explicitly by the programmer.

If a union contains a non-static data member with a non-trivial default constructor, the default constructor of the union is deleted by default unless a variant member of the union has a default member initializer.

At most one variant member can have a default member initializer.

[mrdomino]

Ok - I would want to see some sanity testing actually. I am sure this is fine, but it looks like the old optional was assuming that it would just have a default-constructed value in there at all times, and I'm not yet totally sold on the current assignment operator implementations (relative to a by-value assignment operator that just does swap(rhs).)

[alkis]

Ok - I would want to see some sanity testing actually

Can I run the existing tests with ubsan?

I am running tests like so and they fail on master:

 make o//test/ctl/optional_test.ok

Is this the right way to run them?

the old optional was assuming that it would just have a default-constructed value in there at all time

Yeap the old optional was broken.

ctl::optional<std::string> s;
s.reset(); // destroy the default constructed string
s = std::string("42");  // assignment into a destroyed string (oops)

by-value assignment operator that just does swap(rhs)

This also works but it is generally suboptimal as it involves 3 assignments vs 2 when implemented by hand. Most of the times compiler can elide but not always. If you want I can switch to pass by value and swap.

[jart]

Approved for copyright.

[jart]

Try make m=dbg o/dbg/test/ctl to run all CTL tests under ubsan and asan.

[alkis]

Ran the tests with ubsan and they pass. Also added test that fails in old optional.

[mrdomino]

Looks good. Merge at your leisure.

[mrdomino]

Actually nvm I don't know if you have the power to push the button - I'll push the button.

[mrdomino]

Anyway - thanks for this!

[mrdomino]

by-value assignment operator that just does swap(rhs)

This also works but it is generally suboptimal as it involves 3 assignments vs 2 when implemented by hand. Most of the times compiler can elide but not always. If you want I can switch to pass by value and swap.

We should probably look at this. The only compiler we really care about here (IIUC) is cosmoc++, which is based on gcc, which I'd be surprised if it couldn't elide the assignment. Plus you get to elide a conditional branch if you do it by-value.

[alkis]

surprised if it couldn't elide the assignment. Plus you get to elide a conditional branch if you do it by-value.

We have no control of T so unless there is some rule that guarantees elision we might be out of luck.