Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement optional per C++17 + P0602 #674

Merged
merged 1 commit into from Jun 21, 2017

Conversation

CaseyCarter
Copy link
Collaborator

@CaseyCarter CaseyCarter commented Jun 19, 2017

...because I love implementing optional over and over.

Not quite conforming due to my use of Assignable instead of is_assignable for constraints, since Assignable has a CommonReference requirement, but otherwise passes the libc++ tests.

There's a ton of adjustment necessary to semiregular, which now:

  • conditionally implements assignment as construction
  • value-inits the contained T object when it is default constructible
  • perfectly forwards *this
  • supports semiregular<T &&>

generator changes:

  • semiregular<Reference> now perfectly supports generator_promise for all Reference types
  • eliminate generator_promise_storage from the implementation

@CaseyCarter
Copy link
Collaborator Author

Yes, it takes a ridiculous number of nested classes to implement conditionally trivial special member functions. FOR THE LOVE OF GOD PUT CONCEPTS IN C++20.

@CaseyCarter
Copy link
Collaborator Author

Last change before PR: "Oh, optional_storage has a default member initializer for one of the members of its anonymous union, I can simply = default the default constructor." This then breaks literally every build; even the current compilers (https://godbolt.org/g/GpQptJ) don't yet implement the C++17 change that makes that possible.

@timsong-cpp
Copy link
Contributor

nullopt_t() = delete;

How does this not break optional<int> o; o = {};?

@CaseyCarter
Copy link
Collaborator Author

CaseyCarter commented Jun 19, 2017

How does this not break optional<int> o; o = {};?

It does break it. I should have given more credence to the failing assignment tests and not have written them off as all being due to concept mismatch.

EDIT: Apparently I still haven't implemented optional enough times.

@CaseyCarter
Copy link
Collaborator Author

CaseyCarter commented Jun 19, 2017

I should have given more credence to the failing assignment tests and not have written them off as all being due to concept mismatch.

Now passes all non-hash libc++ tests; except for the test that expects optional<const foo>::value_type to be const foo and not foo. I think it's a defect in the standard that value_type is not remove_cv_t<T>.

EDIT: I should clarify this passes tests only with #define Assignable std::is_assignable to avoid the CommonReference requirement.

{}
T & get()
template<typename T>
struct semiregular_base
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you move the contents of semiregular_base into semiregular, you can eliminate semiregular_base from the design altogether and use optional<T> directly.

@CaseyCarter
Copy link
Collaborator Author

To reviewers: note that semiregular - the class template, not the concept SemiRegular - is substantially larger after this change since it tries expose the semantics of the wrapped type when possible. That means:

  • Default construction value-initializes the object in the optional when the wrapped type is DefaultConstructible; previously default construction always left the optional empty.
  • Copy/move assignment assign directly into the object in the optional when the wrapped type supports copy/move assignment and a contained value exists; previously copy/move assignment always destroyed and emplaced a new contained value.

@CaseyCarter
Copy link
Collaborator Author

CaseyCarter commented Jun 20, 2017

There's a fresh push that:

  • moves the guts of semiregular_base into semiregular and removes semiregular_base from the design
  • Maximizes the use of constexpr in optional, allowing virtually any operation in constexpr context that doesn't require a change of active member in the underlying union. E.g., for optional<literal type>:
    constexpr bool foo() {
        optional<int> o{42};
        o = 13;
        o = {};
        o = 29;
        o.reset();
        o = 42;
        return *o;
    }
    static_assert(foo());

I promise I'll stop messing with the PR now.

@CaseyCarter
Copy link
Collaborator Author

I promise I'll stop messing with the PR now.

I lied. I decided that optional<literal type> would be more useful in constexpr context if empty optionals hold a value-initialized T so that they can switch from disengaged to engaged without changing the active union member. If T is both trivially destructible and trivially default constructible, optional<T> o; initializes o to be an empty optional that contains a value-initialized T.

This difference is user observable via non-trivial special member functions; I should probably constrain it to trivially copyable types as well.

@ericniebler
Copy link
Owner

Last chance for more tweaks, @CaseyCarter. Going once, going twice ...

...because I love implementing optional over and  over.

Not quite conforming due to my use of Assignable instead of is_assignable for constraints, since Assignable has a CommonReference requirement, but otherwise passes the libc++ tests.

There's a ton of adjustment necessary to semiregular, which now:
* *conditionally* implements assignment as construction
* value-inits the contained T object when it is default constructible
* perfectly forwards *this
* supports semiregular<T &&>

generator changes:
* semiregular<Reference> now perfectly supports generator_promise for all Reference types
* eliminate generator_promise_storage from the implementation
@CaseyCarter
Copy link
Collaborator Author

Now that you mention it...

semiregular<Reference> now has exactly the semantics generator_promise<Reference> needs, so I can greatly simplify the implementation of generator_promise.

@ericniebler ericniebler merged commit dc672ec into ericniebler:master Jun 21, 2017
@ericniebler
Copy link
Owner

Big improvement, thanks Casey.

ericniebler added a commit that referenced this pull request Jun 21, 2017
Implement optional per C++17 + P0602
@CaseyCarter CaseyCarter deleted the optional branch June 21, 2017 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants