This is an archive of the discontinued LLVM Phabricator instance.

Implement N4606 optional
ClosedPublic

Authored by EricWF on Jul 24 2016, 6:31 PM.

Details

Summary

Adapt implementation of Library Fundamentals TS optional into an implementation of N4606 optional.

Audit P/Rs for optional LWG issues:

  • 2756 "C++ WP optional<T> should 'forward' T's implicit conversions" Implemented, which also resolves 2753 "Optional's constructors and assignments need constraints" (modulo my refusal to explicitly delete the move operations, which is a design error that I'm working on correcting in the 2756 P/R).
  • 2736 "nullopt_t insufficiently constrained" Already conforming. I've added a test ensuring that nullopt_t is not copy-initializable from an empty braced-init-list, which I believe is the root intent of the issue, to avoid regression.
  • 2740 "constexpr optional<T>::operator->" Already conforming.
  • 2746 "Inconsistency between requirements for emplace between optional and variant" No P/R, but note that the author's '"suggested resolution" is already implemented.
  • 2748 "swappable traits for optionals" Already conforming.
  • 2753 "Optional's constructors and assignments need constraints" Implemented.

Most of the work for this patch was done by Casey Carter @ Microsoft. Thank you Casey!

Diff Detail

Event Timeline

CaseyCarter retitled this revision from to Implement N4606 optional.
CaseyCarter updated this object.
CaseyCarter added a reviewer: EricWF.
EricWF edited edge metadata.Jul 27 2016, 1:53 PM

We should consider adding a storage specialization for trivially copyable types. It's important to do it now because it would be an ABI breaking change later. It would also be possible to provide constexpr optional<T> copy/move operations for such a specialization.

More comments to come.

include/optional
175

I really don't like this constructor. I can see plenty of places where it could get constructed accidentally.
Let's find another way to (A) get rid of the default constructor and (B) privately initialize nullopt_t.

202

Or just __null_state_(0)

259

I've never seen variable template specialization, but I like it :-)

346

It seems like this implementation should support references fine, no? It might be one of those cases that needs std::launder, but __optional_storage looks like it could handle it.

Do you know what the status of optional<T&>is? Is LEWG working on it?

420

There are not constexpr in the spec, and it can't be implemented as a constexpr operation in the vast majority* of cases AFAIK.

[1] http://stackoverflow.com/questions/30632442/c14-constexpr-union-conditional-initialization-in-constructor

We should consider adding a storage specialization for trivially copyable types. It's important to do it now because it would be an ABI breaking change later. It would also be possible to provide constexpr optional<T> copy/move operations for such a specialization.

Intent of this change was to perform the minimum changes necessary to bring the existing LibFund optional implementation into spec with the WP, so I didn't mess about with the existing design at all, really. That said, I put a bit of effort into providing trivial-and-therefor-constexpr copy/move constructors/assignments for MSVC's optional implementation, so I obviously agree it's a good idea. If you're interested I'd be happy to implement it here as well.

include/optional
175

How about an unspeakable tag argument, e.g.:

struct nullopt_t
{
    struct __secret_tag { explicit __secret_tag() = default; };
    explicit constexpr nullopt_t(__secret_tag, __secret_tag) noexcept {}
};

constexpr nullopt_t nullopt{nullopt_t::__secret_tag{}, nullopt_t::__secret_tag{}};
202

I would have simply value-initialized; I didn't change anything pre-existing that was not necessary to accommodate the spec changes from TS2 to WP optional.

'E' for "empty" would be a good choice as well. It's a shame "Ø" isn't in the basic source character set. ;)

210

There are *four* virtually identical constructors with this same pattern; consider refactoring.

259

I use it *very* extensively in cmcstl2 to workaround performance deficiencies in GCC's concepts implementation, so I've enjoyed riding a roller coaster of bugs for the last year. I can now confidently say that GCC & Clang both have good support for partial specialization of variable templates.

346

It's an explicit requirement in N4606 that optional references are verboten. Yes, that is inconsistent with variant which explicitly *does* support references. I don't know if anyone has been working on it. Hopefully we'll get the differences ironed out sometime before C++20.

Psuedo-destructor syntax isn't valid for reference types, so e.g. __optional_storage's destructor would need a bit of work to support them.

I would want a specialized representation for optional<T&> that stores only a pointer and uses nullptr for the empty state so the general implementation of __optional_storage wouldn't need to support them anyway.

420

These *are* constexpr in Library Fundamentals TS2 (para 44 & 49) just as specified in the resolution of LWG2451. I agree they cannot be implemented as constexpr; I submitted LWG2745 to remove the constexpr and "shall be constexpr if ..." wording.

CaseyCarter updated this object.
CaseyCarter edited edge metadata.

Address review comments:

  • Defend nullopt_t constructor against Murphy.
  • Refactor __optional_storage into three class templates:
    • __optional_destruct_base: conditionally implements non-trivial destruction
    • __optional_copy_base: conditionally implements non-trivial copy/move construction/assignment
    • __optional_storage: presents a clean API to insulate optional from the storage representation. (In theory, specializations of this class can provide novel representations, e.g., optional<T&> could use a T* for its representation with (T*)0 as the empty state.)
CaseyCarter added inline comments.Jul 29 2016, 10:35 AM
include/optional
315

Where are the tests to validate trivial/constexpr move/copy construct/assign for optional of trivial types?

optional
2 ↗(On Diff #66142)

It bears repeating. An implementation just can't have too many files named "optional."

Delete this and re-push.

Remove the extra "optional" file that I somehow managed to add at the top of the directory tree.

NOTE WELL: This implementation of the "trivially copyable type" optimization critically fails to SFINAE the special member functions. I suggest you put off reviewing until I update the diff again.

include/optional
316

Those tests also validate that the special members don't participate in overload resolution when they're not valid for T, which I haven't implemented here, and is IMO critical for optional to be useful (LWG2753).

Adding a bunch of inline comments. This looks really great. I should finish the review tommorow.

The biggest problem, which you already noted, is that the special members still lack SFINAE. We need to make sure they are correctly generated. Here's the test case I wrote for it.

include/experimental/optional
11 ↗(On Diff #66145)

Let's go with the full _LIBCPP_EXPERIMENTAL_OPTIONAL.

include/optional
144

bad_optional_access should go directly into namespace std, not the versioning namespace, because we don't version exceptions.

159

Just put these includes at the top. If you include <optional> when it's not available then it's your fault. We don't need to optimize for it.

167

Put this right after the first includes.

320

What's up with this default template argument?

406

I don't think we want to check this. Sometime's the implicitly generated destructor is not noexcept.

409

= default;?

412

Don't even declare the destructor.

416

I think we probably want to constrain these or is_constructible<value_type, value_type&&> even though the standard doesn't require it.

784

Urg this is unfortunate. Clang provides a constexpr addressof, but GCC doesn't yet.

Even worse is that this technique doesn't detect explicitly deleted operator& overloads.

803

Calls to declval *need* to be qualified to avoid ADL hijacking.

1168

decay_t is now available :-)

1203

_LIBCPP_STD_VER > 14

test/libcxx/optional/version.pass.cpp
1 ↗(On Diff #66145)

test/libcxx/utilities/optional is the correct path for this.

CaseyCarter marked 11 inline comments as done.Aug 11 2016, 9:32 AM

I should have time to finish up this week.

include/optional
320

I foresee wanting to specialize __optional_storage for types with a built-in invalid state / sentinel value some time, especially if/when the committee decides to allow optional<T&>. This parameter is here *now* so that when that time comes it can be used for selecting specializations without ABI breaking.

406

This was pre-existing in experimental/optional, but I agree. Will remove.

409

This was pre-existing, and it confused me as well, until I recalled [dcl.init]/7: "If a program calls for the default-initialization of an object of a const-qualified type T, T shall be a class type with a user-provided default constructor." I see no reason not to allow e.g. constexpr optional<foo> o;

416

Yes, constraining the constructors and assignments is LWG2753. I'll be sure to implement all the issues that are Tentatively Ready from Chicago.

CaseyCarter marked an inline comment as done.

Implements the most recent PR for LWG2756 - which also resolves LWG2753 - except that optional<T>'s move operations are not defined as deleted when T is not movable. They instead do not participate in overload resolution. Copyable-but-not-Movable types are an abomination that optional should not propagate. Consequently, optional<T> is both copyable and movable when T is Copyable-but-not-Movable; this is a feature, not a bug ;)

I split Eric's special-member-function triviality/SFINAE test into separate tests:

  • An SFINAE test under std, since LWG2753 is Tentatively Ready
  • A triviality test under libcxx, since no one has shown an interest in standardizing the triviality of optional<T>'s SMFs.

Implemented a test for LWG2736 "nullopt_t insufficiently constrained." nullopt_t already conformed, but it never hurts to guard against regression.

Wrapped all the everything at 100-ish columns.

CaseyCarter marked 5 inline comments as done.Aug 13 2016, 10:42 PM

That's everything I'm planning to do for now. Review away.

include/optional
785

I can live with this nearly-pathological corner case being broken without intrinsic addressof.

Fix triviality impl and tests (you know, because you tend to find errors when you actually *run* the tests)

  • Add trivially_assignable tests
  • Cleanup:
    • add some missing _LIBCPP_INLINE_VISIBILITY specifiers
    • We're inside _LIBCPP_STD_VER > 14, so use trait_v everywhere instead of trait::value.
    • Use is_trivially_copyable<T> directly, instead of the equivalent but longer is_trivially_copyable<__optional_destruct_base<T>>.
    • "inline constexpr" => "constexpr"
CaseyCarter marked 6 inline comments as done.Aug 14 2016, 4:58 PM

Nitpicks.

include/optional
168

Why is "Access" capitalized?

648

Again, you've written four nearly identical constructor bodies. This cries out for DRY.

test/libcxx/utilities/optional/optional.object/special_member_gen.pass.cpp
38

Needs to check triviality of copy/move assignment as well.

Apply Eric's refactoring to separate the SFINAE constraints from the implementation of the special member functions.

CaseyCarter added inline comments.Aug 14 2016, 6:35 PM
include/optional
389

Note the intentional difference from Eric's patch: this class deletes the *assignments* as well as the constructions when T is not movable and/or copyable, so that optional<T> is foo_assignable iff T is foo_constructible and foo_copyable.

Given that constructibility partially determines assignment, it may be possible to combine the two sfinae bases cleanly.

Hi Casey? Sorry about this, but I actually just factored out the __optional_sfinae_ctor_base and __optional_sfinae_assign_base since we'll need them in optional. Could you please apply this patch to yours?

https://gist.github.com/EricWF/49ffb06a2ac904b5f4729c1e72725fbe

include/optional
168

Just make it "bad optional access". I don't think diagnostics should ever be capitalized.

321

I suspect we could make the change is a mostly non-abi breaking change without it, but I'm OK with it in general.
Just add a comment along the lines of "For future specialization."

648

I think we could just lower it to an __optional_storage constructor using

optional(optional<U>&& u) : __base(std::move(u)) {}

or similar

Additionally I'm perfectly happy to take over this change if you don't want to continue. If you do then thank you very much! I understand it's a lot of work.

EricWF added inline comments.Aug 14 2016, 7:06 PM
include/optional
752

Please replace this with:

if (!this->has_value())
  __libcpp_throw(bad_optional_access());
764

Same here.

776

Same here.

788

Same here.

CaseyCarter added inline comments.Aug 14 2016, 7:08 PM
include/optional
389

Given that constructibility partially determines assignment, it may be possible to combine the two sfinae bases cleanly.

This results in 9 specializations of 1 class instead of 4 specializations * 2 classes. I'd call it a wash.

474

I moved these here so there would be one private section, but I'm now thinking it would be cleaner to make __base public and move these __operator_arrow overloads back into a private section at the end of the class body.

EricWF added inline comments.Aug 14 2016, 7:09 PM
include/optional
389

I actually changed how it was done. Now __optional_sfinae_assign_base_t selects the correct copy/assignment operator definitions based on the copy/move constructor availability. Sorry I missed that in the first patch.

CaseyCarter marked 4 inline comments as done.Aug 14 2016, 7:13 PM

Hi Casey? Sorry about this, but I actually just factored out the __optional_sfinae_ctor_base and __optional_sfinae_assign_base since we'll need them in optional. Could you please apply this patch to yours?

I could, but then the tests won't run. Could you include the destination of the refactoring in the patch as well?

include/optional
322

Already removed during the sfinae refactor - whoever specializes optional_storage for references will have to specialize it separately for lvalue and rvalue references ;)

649

Already done.

Hi Casey? Sorry about this, but I actually just factored out the __optional_sfinae_ctor_base and __optional_sfinae_assign_base since we'll need them in optional. Could you please apply this patch to yours?

I could, but then the tests won't run. Could you include the destination of the refactoring in the patch as well?

I already pushed the destination to master. The definitions are at the bottom of __tuple.

CaseyCarter marked 5 inline comments as done.

Relocate __operator_arrow; __libcpp_throw; __sfinae_XXX_base relocation.

The summary says:

Speculatively apply resolution of http://wg21.link/lwg2451 from the TS

It we want to apply the defect report for the WP, not the TS. IE:

  1. C++ WP optional<T> should 'forward' T's implicit conversions
EricWF added inline comments.Aug 14 2016, 8:35 PM
include/optional
738

These need non-const overloads as well.

Side note: The current patch make clang ICE a bunch. I've filed a bug against Clang here: https://llvm.org/bugs/show_bug.cgi?id=28978

This bug doesn't seem to affect 3.9 or 3.8 so we shouldn't have a real issue.

CaseyCarter marked an inline comment as done.Aug 14 2016, 10:06 PM

It we want to apply the defect report for the WP, not the TS.

Yes, I implemented 2756 today and did a full audit of all the active optional issues, but forgot to update the Summary.

__operator_arrow needs to work for *mutable* objects as well as constant objects.

@CaseyCarter I've updated the implementation for references according to your feedback. optional<T&> and optional<T&&> now have sizeof(void*). What do you think of this patch:

https://gist.github.com/EricWF/a43e98641c3270b411a9358601bc2f0d

EricWF commandeered this revision.Oct 12 2016, 12:03 AM
EricWF updated this revision to Diff 74333.
EricWF edited reviewers, added: CaseyCarter; removed: EricWF.

Updating with the final revision.

EricWF updated this object.Oct 12 2016, 12:48 AM
EricWF accepted this revision.Oct 12 2016, 12:52 AM
EricWF updated this revision to Diff 74335.
EricWF added a reviewer: EricWF.
This revision is now accepted and ready to land.Oct 12 2016, 12:54 AM
EricWF closed this revision.Oct 12 2016, 12:55 AM