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

Prevent calling make_optional<int&>(i) or make_optional<const int&>(i) #59

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

Quuxplusone
Copy link
Contributor

The status quo in C++23 is that

    int i;
    auto o = make_optional<int&>(i);

produces an optional<int>, not an optional<int&>. This would be horribly confusing to propagate into C++26 — users would naturally expect to receive an optional<int&>. Worse,

    auto o = make_optional<int&>(std::ref(i));

actually would give you an optional<int&>, because it would use a different overload of make_optional! So, we propose to add a "Mandates" element to make_optional that requires its explicit template argument (if any) to be a non-array object type.
This breaks the "status quo" example at the top of this comment (requiring an Annex C entry); but it successfully prevents the pitfalls. And it does not silently change the behavior of the "status quo" example, either — which would probably be 100x worse than merely breaking it!

Thanks to Tomasz Kamiński for the template-programming trick, which matches the technique used in https://eel.is/c++draft/out.ptr .

`static_cast` to lvalue suppresses Clang's self-assignment warning.
Add (missing?) EXPECTs to silence Clang's unused-variable warning.
Add an accessor to suppress Clang's unused-private-field warning.
Drive-by rename "empty_base" to "disengaged_base", since the phrase
"empty base" means something different in C++.
Add two new tests: one for `optional<optional<int>&>`, and one
testing that we can construct and assign to `optional<T&>` from
`reference_wrapper<T>` (which should work: since `reference_wrapper<T>`
is implicitly convertible to `T&`, it also should be implicitly
convertible to `optional<T&>`).
These tests are both red before this patch and green afterward.
The new tests are red before the patch and green afterward.
The return type of `emplace` had been wrong; `emplace` returns a
reference to the emplaced value (in this case, the T& reference
that was bound), not `*this`.

The new tests are red before the patch and green afterward.
@frederick-vs-ja
Copy link
Contributor

How about making the first overload like this, which seems simpler to me?

    namespace detail {
        struct spec_barrier {
            explicit spec_barrier() = default;
        };
    }

    template<detail::spec_barrier = detail::spec_barrier{}, class T>
        requires std::is_constructible_v<std::decay_t<T>, T> // per LWG3627
    constexpr std::optional<std::decay_t<T>> make_optional(T&& t)
        noexcept(std::is_nothrow_constructible_v<std::decay_t<T>, T>)
    {
        return std::optional<std::decay_t<T>>(std::forward<T>(t));
    }

This test fails with at least one alternative approach to the
constructor overload set (involving =delete'd candidates which
make this overload resolution ambiguous).
The status quo in C++23 is that
    int i;
    auto o = make_optional<int&>(i);
produces an `optional<int>`, not an `optional<int&>`. This would be horribly
confusing to propagate into C++26 — users would naturally expect to receive
an `optional<int&>`. Worse,
    auto o = make_optional<int&>(std::ref(i));
actually *would* give you an `optional<int&>`, because it would use a different
overload of `make_optional`! So, we propose to add a "Mandates" element to
`make_optional` that requires its explicit template argument (if any) to be
a non-array object type.
This breaks the "status quo" example at the top of this comment (requiring
an Annex C entry); but it successfully prevents the pitfalls.

Thanks to Tomasz Kamiński for the template-programming trick, which matches
the technique used in https://eel.is/c++draft/out.ptr .
@Quuxplusone
Copy link
Contributor Author

How about making the first overload like this, which seems simpler to me?

Oh yeah — that works, because even though today the first overload is used for e.g. make_optional<int&>(i), there's no reason it needs to be. We can make the first overload non-viable in that case, and then the second overload will be used instead, which is fine! (Until someone points out a subtle ABI problem with that change, anyway... ;))

Updated accordingly.

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.

2 participants