[CXX-2625] Bring our own optional<T> #1075

vector-of-bool · 2024-01-05T06:54:52Z

This PR is a continuation of the work for CXX-2625. The PR itself contains many commits from the string_view PR, since this was created from that work. The relevant commits for optional<T> begin with 947d289.

…ment

…tors mixin

src/bsoncxx/include/bsoncxx/v_noabi/bsoncxx/stdx/optional.hpp

src/bsoncxx/include/bsoncxx/v_noabi/bsoncxx/config/util.hpp

src/bsoncxx/test/CMakeLists.txt

src/bsoncxx/include/bsoncxx/v_noabi/bsoncxx/stdx/optional.hpp

kevinAlbs · 2024-02-05T16:02:56Z

src/bsoncxx/test/optional.test.cpp

+    optional<std::string> string2 = std::move(c_str);
+    CHECK(string == c_str);
+    CHECK(string2 == c_str);
+}


Suggest adding more exhaustive tests of use the new API. Example: https://gist.github.com/kevinAlbs/5f3ddccb491f5e4c42633589781e1909

Tests may have helped revealed earlier issues (e.g. converting move constructors and swap). These tests may help cross-reference with cppreference to determine any missing API.

Added many more cases, including those from your gist. Should get better coverage now.

eramongodb

Thank you for your hard work on this PR. Some minor suggestions remaining; otherwise, LGTM.

src/bsoncxx/include/bsoncxx/v_noabi/bsoncxx/stdx/optional.hpp

eramongodb · 2024-02-08T22:06:57Z

src/bsoncxx/include/bsoncxx/v_noabi/bsoncxx/stdx/version.hpp

+#pragma once
+
+#define BSONCXX_FEATURE_HaveCxxStdlibVersionMacros -
+#ifdef __has_include
+#if __has_include(<version>)
+#include <version>
+#undef BSONCXX_FEATURE_HaveCxxStdlibVersionMacros
+#define BSONCXX_FEATURE_HaveCxxStdlibVersionMacros +
+#endif
+#endif


Suggested change

#pragma once

#define BSONCXX_FEATURE_HaveCxxStdlibVersionMacros -

#ifdef __has_include

#if __has_include(<version>)

#include <version>

#undef BSONCXX_FEATURE_HaveCxxStdlibVersionMacros

#define BSONCXX_FEATURE_HaveCxxStdlibVersionMacros +

#endif

#endif

Remove stray, unused header. (Added unintentionally?)

eramongodb · 2024-02-08T22:12:47Z

src/bsoncxx/test/optional.test.cpp

+    CHECK(hashit(a) == hashit(b));
+    b.emplace(41);
+    CHECK(hashit(41) == hashit(b));
+    CHECK(hashit(a) != hashit(b));  // (Extremely probable, but not certain)


Suggested change

CHECK(hashit(a) != hashit(b)); // (Extremely probable, but not certain)

CHECK_NOFAIL(hashit(a) != hashit(b)); // (Extremely probable, but not certain)

Consider using CHECK_NOFAIL for consistency with the "not certain" description.

kevinAlbs

LGTM with one possible added condition and minor error message fixups.

kevinAlbs · 2024-02-07T15:57:10Z

src/bsoncxx/include/bsoncxx/v_noabi/bsoncxx/stdx/optional.hpp

+struct enable_opt_value_conversion   //
+    : bsoncxx::detail::conjunction<  //
+          std::is_constructible<To, From&&>,
+          bsoncxx::detail::negation<bsoncxx::detail::is_alike<From, in_place_t>>,


https://en.cppreference.com/w/cpp/utility/optional/optional notes this condition for (8):

std::decay_t<U>(until C++20) std::remove_cvref_t<U>(since C++20) is neither std::in_place_t nor std::optional<T>

Is checking for std::optional<T> also needed? Example:

bsoncxx::detail::conjunction< bsoncxx::detail::negation<bsoncxx::detail::is_alike<From, in_place_t>>, // bsoncxx::detail::negation<bsoncxx::detail::is_alike<From, optional<To>>> // >,

I am not sure it is needed. I was unable to come up with a test case that showed any behavior difference.

kevinAlbs · 2024-02-08T16:24:00Z

src/bsoncxx/include/bsoncxx/v_noabi/bsoncxx/stdx/optional.hpp

+        return this->_storage.value;
+    }
+    bsoncxx_cxx14_constexpr rvalue_reference operator*() && noexcept {
+        _assert_has_value("operator*() &");


Suggested change

_assert_has_value("operator*() &");

_assert_has_value("operator*() &&");

kevinAlbs · 2024-02-08T16:24:07Z

src/bsoncxx/include/bsoncxx/v_noabi/bsoncxx/stdx/optional.hpp

+        return static_cast<rvalue_reference>(**this);
+    }
+    bsoncxx_cxx14_constexpr const_rvalue_reference operator*() const&& noexcept {
+        _assert_has_value("operator*() const&");


Suggested change

_assert_has_value("operator*() const&");

_assert_has_value("operator*() const&&");

vector-of-bool added 30 commits November 10, 2023 14:35

Shorthand for trailing return type, noexcept, and single-return-state…

71f0e61

…ment

Some additional traits: decay_copy and rank<>

f5e1d02

New operators.hpp utilities

f79efdb

Iterator utilities

ff488e3

We don't need to start_mongod for the simple compile tasks

155f528

A new set of macros for concise diagnostic controls

e3575fa

Disable warnings about void-dereference on Clang

fb1ac46

Reorganize macro definitions

41d6765

Fix: malformed pragmas on MSVC

3868e35

A small amount of std::ranges from the future

c7350d3

Bring our own std::string_view

c13e052

Avoid warnings about "unsecure stdlib" code

f2bb2fb

Tweak definition of is_equality_comparable

fbd5535

decay_copy is no longer needed (yet)

5740433

Check for MSVC's version macro

b318f62

No ADL-only data()

ff7978d

Remove c_str(), tweak "string-like" detection

4aeae1d

Minor spelling and tests

536fc05

Merge branch 'master' into own-string_view.2

b973270

MSVC 19.10 lies about C++14 constexpr

39dc524

Test and fixup for ADL lookup on VS 2015

d67f942

Cleaner size() constraint

c1a7997

unreachable_sentinel, plus support for arg swapping in equality_opera…

40f544f

…tors mixin

Qualify test names in string_view tests

e083ca5

Bring our own algorithm.hpp

43c7924

Implement find() and contains() from string_view

09961a8

Merge branch 'master' into own-string_view.2

e9e78ea

Remove newer .clang-foramt option, document internal macros as internal

1f3c509

Calm linter

67af4b3

Sort file lists

2c61f8d

vector-of-bool added 3 commits January 30, 2024 16:52

Merge branch 'master' into own-optional

bf1a6b0

Workaround for MSVC weirdness

452f9d7

AppleClang also lacks LWG2543

1e9c20e

vector-of-bool requested review from eramongodb and kevinAlbs February 1, 2024 19:57

eramongodb requested changes Feb 2, 2024

View reviewed changes

kevinAlbs reviewed Feb 5, 2024

View reviewed changes

vector-of-bool added 8 commits February 5, 2024 13:43

Exhaustic relation checks, remove stray line

47336c2

Fix optional<const T>

51b4273

Fix noexcept spec for optional swap

8e4fd82

Fix hash to match behavior of std::optional

0c0e370

Inline reversed swap

2ee8b0c

Many more test cases

0ee25ca

Make nullopt_t non-aggregate

abd8aa2

Shift position of macro manipulations

ff54bdf

kevinAlbs self-requested a review February 5, 2024 21:31

vector-of-bool added 4 commits February 5, 2024 14:58

Ensure optional.hpp is included first in its test case

017696f

Fix warnings and tests for opt conversion

6dd22a2

No conversion warning

b4196f3

Fix: compare_three_way name lookup differences between compilers

feaaa16

vector-of-bool requested a review from eramongodb February 8, 2024 20:04

eramongodb reviewed Feb 8, 2024

View reviewed changes

eramongodb approved these changes Feb 8, 2024

View reviewed changes

kevinAlbs approved these changes Feb 9, 2024

View reviewed changes

vector-of-bool added 4 commits February 9, 2024 12:58

Unused header

5723a66

Minor PR tweaks

4eddb46

Merge branch 'master' into own-optional

c61402c

fmt tweak

c3b1646

vector-of-bool merged commit e21084e into mongodb:master Feb 12, 2024

eramongodb mentioned this pull request Nov 7, 2024

CXX-2797 Remove support for external polyfill libraries #1257

Merged

	CHECK(hashit(a) != hashit(b)); // (Extremely probable, but not certain)
	CHECK_NOFAIL(hashit(a) != hashit(b)); // (Extremely probable, but not certain)

	_assert_has_value("operator*() &");
	_assert_has_value("operator*() &&");

	_assert_has_value("operator*() const&");
	_assert_has_value("operator*() const&&");

[CXX-2625] Bring our own optional<T> #1075

[CXX-2625] Bring our own optional<T> #1075

Uh oh!

Conversation

vector-of-bool commented Jan 5, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevinAlbs Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

vector-of-bool Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

eramongodb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eramongodb Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

eramongodb Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

kevinAlbs Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

kevinAlbs Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

kevinAlbs Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants