CXX-3232 add bsoncxx v1 declarations #1412
Conversation
eramongodb
force-pushed
the
cxx-abi-v1-decls
branch
2 times, most recently
from
b52a537 to
07f460e
Compare
eramongodb
force-pushed
the
cxx-abi-v1-decls
branch
from
07f460e to
98521ae
Compare
| if (("$ret" & 0x03)); then # ABIDIFF_ERROR (1) | ABIDIFF_USAGE_ERROR (2) | ||
| echo "abidiff error" >&2 | ||
| exit 1 | ||
| elif (("$ret" & 0x08)); then # ABIDIFF_ABI_INCOMPATIBLE_CHANGE (8). |
There was a problem hiding this comment.
Per abidiff docs:
The exit code of the abidiff command is either
0if the ABI of the binaries being compared are equal, or non-zero if they differ or if the tool encountered an error. In the later case, the exit code is a 8-bits-wide bit field in which each bit has a specific meaning.
This PR introduces the first exported v1 ABI symbol: bsoncxx::v1::exception::~exception(). This new symbol triggered a non-zero exit code of ABIDIFF_ABI_CHANGE (4), which was incorrectly handled as incompatibility/failure. The script is updated to distinguish the individual error codes appropriately: we only care about incompatible changes. Compatible changes (such as this) will still be included in the resulting abidiff report.
| std::is_trivially_copy_constructible<T>, | ||
| std::is_trivially_copy_assignable<T>> {}; | ||
| struct is_semitrivial | ||
| // https://gcc.gnu.org/onlinedocs/gcc-4.9.0/libstdc++/manual/manual/status.html |
There was a problem hiding this comment.
Unfortunately, GCC 4.X (used by RHEL 7) does not support the std::is_trivially_* type traits. Special-case this compiler version by simply assuming this property to be true (we have plenty of true coverage with other compilers and compiler versions on other platforms).
| static_assert( | ||
| is_nothrow_moveable<view::const_iterator>::value, | ||
| "bsoncxx::v1::document::view::const_iterator must be nothrow moveable"); |
There was a problem hiding this comment.
Despite v1::document::view being semitrivial, its iterator is only nothrow moveable due to its internal v1::element::view data member.
| static_assert( | ||
| is_nothrow_moveable<view::const_iterator>::value, | ||
| "bsoncxx::v1::array::view::const_iterator must be nothrow moveable"); |
There was a problem hiding this comment.
Despite v1::array::view being semitrivial, its iterator is only nothrow moveable due to its internal v1::element::view data member.
| namespace bsoncxx { | ||
| namespace v1 { | ||
|
|
||
| exception::~exception() = default; |
There was a problem hiding this comment.
This is the key function for the bsoncxx::v1::exception class and the first v1 ABI symbol (along with the vtable) to be exported by the bsoncxx library. 🎉
Although this is meant a declarations-only PR, this definition is required to support successful builds with MSVC, as otherwise:
error LNK2019: unresolved external symbol "public: virtual __cdecl bsoncxx::v1::exception::~exception(void)" (??1exception@v1@bsoncxx@@UEAA@XZ) referenced in function "public: virtual void * __cdecl bsoncxx::v1::exception::`vector deleting destructor'(unsigned int)" (??_Eexception@v1@bsoncxx@@UEAAPEAXI@Z)
| deleter_type const& get_deleter() const { | ||
| return _data.get_deleter(); | ||
| } |
There was a problem hiding this comment.
A non-const .get_deleter() is deliberately not provided to prevent "desync" of the deleter from the associated BSON binary data. Read-only access is nevertheless essential to support inspection of the type of underlying deleter, such as for v_noabi compatibility (e.g. "Is this deleter compatible with v_noabi::document::value::deleter_type?"). This will be explored in CXX-3234 (v_noabi refactor).
| /// | ||
| /// Implicitly convert to `this->view()`. | ||
| /// | ||
| /* explicit(false) */ operator v1::document::view() const { | ||
| return this->view(); | ||
| } |
There was a problem hiding this comment.
Asymmetry: value -> view is implicit, but view -> value is explicit.
| /// | ||
| /// Implicitly convert to `this->view()`. | ||
| /// | ||
| /* explicit(false) */ operator v1::types::view() const { | ||
| return this->view(); | ||
| } |
There was a problem hiding this comment.
Asymmetry: value -> view is implicit, but view -> value is explicit.
| /// | ||
| /// Return a view of the BSON binary data as a document. | ||
| /// | ||
| v1::document::view view() const { | ||
| return {_data.get(), _length}; | ||
| } |
There was a problem hiding this comment.
All member functions below are defined in terms of v1::document::view, including error codes and Doxygen documentation.
Important
The new "invalid" state of a v1::document::view allows this conversion to be well-defined even when a value is default-initialized (_data.get() == nullptr).
| class value {}; | ||
| class value { | ||
| private: | ||
| v1::document::value _value; |
There was a problem hiding this comment.
All behavior for this class is defined in terms of v1::document::value or v1::array::view, including error codes and Doxygen documentation.
eramongodb
requested review from
rcsanchez97,
kevinAlbs and
vector-of-bool
and removed request for
rcsanchez97
eramongodb
changed the title
| /// @note This iterator almost satisfies Cpp17ForwardIterator, but `std::iterator_traits<T>::reference` is defined as | ||
| /// `value_type`, similar to `std::vector<bool>::iterator` and `std::istreambuf_iterator<T>`. Therefore, this iterator | ||
| /// only fully satisfies Cpp17InputIterator. |
There was a problem hiding this comment.
Update: renamed Legacy -> Cpp17 for consistency with standard spec for C++20 and newer (by P0896R4; cppreference still uses "Legacy", a leftover from when the C++20 spec was still being drafted) + restoring this comment due to the associated changes of the prior comment being lost by GitHub.
The v1 ABI interfaces address and clarify several issues with iterators that were identified during work on CXX-3082:
- Missing const qualifiers on multiple member functions, e.g.
auto const iter = doc.begin(); use(*iter);is ill-formed (?!). iterator_category = std::forward_iterator_tagdespite not actually satisfying the requirements of LegacyForwardIterator:- e.g. does not satisfy
it: const_iterator->*it -> T const&. - e.g. does not satisfy
a == b->std::addressof(*a) == std::addressof(*b).
- e.g. does not satisfy
- Operations on an end iterator behavior, such as deference yielding an invalid element and increment yielding another end iterator, are
underdocumentednot documented at all.
v1::document::view::const_iterator (and v1::array::view::const_iterator) addresses all these issues by ensuring it properly satisfies and declares itself to be a LegacyInputIterator instead (even though it may provide some features of a LegacyForwardIterator).
| /// | ||
| /// Zero-initialize the byte representation. | ||
| /// | ||
| /// @note The result is equivalent to `"0E-6176"`, not `"0"`. |
There was a problem hiding this comment.
I would rather avoid a subtle quiet behavior change between v_noabi and v1. LGTM as proposed.
| /// | ||
| /// Initialize with `code` and `scope`. | ||
| /// | ||
| b_codewscope(v1::stdx::string_view code, v1::document::view scope) : code{code}, scope{scope} {} |
There was a problem hiding this comment.
consider adding a
code-only constructor
I support leaving as-is as a minor simplification since the code-with-scope BSON type is deprecated.
| /// | ||
| /// Initialize with `increment` and `timestamp`. | ||
| /// | ||
| b_timestamp(std::uint32_t increment, std::uint32_t timestamp) : increment{increment}, timestamp{timestamp} {} |
There was a problem hiding this comment.
consider adding a
increment-only constructor
I support leaving it out (as proposed) unless there is a known need.
|
|
||
| public: | ||
| /// | ||
| /// Initialize as an empty view. |
There was a problem hiding this comment.
Confirming: is v1::document::view initialized to an empty document intended for consistency with v_noabi::document::view?
If there were no precedent, I might rather the initialize to an invalid, but I expect that is a quiet behavior change that might be missed when migrating. So I support this even if it means a slight asymmetry between default constructed v1::document::value (invalid) and default constructed v1::document::view (empty).
There was a problem hiding this comment.
It is both for compatibility with v_noabi::document::view as well as consistency with std::string_view, which is default-initialized as an "empty string" rather than an "invalid string view".
It follows that for consistency with std::string, a default-constructed v1::document::value should also be an empty document rather than invalid. However, as noted in CXX-939 and related discussions, this complicates the allocation strategy for representing an empty BSON document (uncondition allocation? use pre-allocated storage? etc.).
We could consider leaving the currently-proposed v1::document::value semantics the same, but make the following tweak to the behavior of this->view():
v1::document::view view() const {
if (v1::document::view const v{_data.get(), _length}) {
return v;
}
return {};
}The result of this tweak would be that a default-initialized value is equivalent to a default-initialized view (both an "empty document") even when value._data.get() == nullptr. No unconditional or special-cased allocation is required. Since all non-ownership-related accessors are implemented in terms of this->view(), the null pointer would not be "observable" to the user via normal BSON document operations except via this->release(). However, this PR did not initially propose this due concern that this may be more confusing to users than the current inconsistency between view vs. value default initialization.
Thoughts?
There was a problem hiding this comment.
I am slightly in favor of keeping as-is. I expect the alternative could cause confusion with release:
auto fn = [](v1::document::value doc) {
auto len = doc.length(); // 5 for both.
auto ptr = doc.release(); // null if default. non-null if empty-constructed.
};
fn(v1::document::value{}); // default.
uint8_t data[5] = {5, 0, 0, 0};
fn(v1::document::value{data, 5}); // empty-constructed.IIUC the linked reports in CXX-939 (1 and 2) wanted to default construct a document::value and assign to it. It may be not commonly needed to use a default constructed document::value as an empty document.
That said, I see the argument for consistency with std::string / std::string_view. I would not object to the alternative.
There was a problem hiding this comment.
IIUC the linked reports in CXX-939 (1 and 2) wanted to default construct a document::value and assign to it. It may be not commonly needed to use a default constructed document::value as an empty document.
I agree. The motivating use cases are common C++ patterns such as:
auto docs = std::vector<bsoncxx::document::value>(n);
docs[0] = ...;and:
// error: no matching constructor for initialization of 'bsoncxx::v_noabi::document::value'
bsoncxx::document::value doc;
if (...) {
doc = ...;
}If we permitted default construction of value, we would need to do one of the following:
- Allow it to point to nothing (nullptr), which would break the invariant that it owns a region of memory, as well as the invariant that it contains BSON that you can view.
- Allocate 5 bytes on the heap to hold an empty BSON document. That would retain the invariants, but it would mean that simply declaring a document::value on the stack would involve a heap allocation. In most cases, that heap allocation would be wasted, as in your example where you almost always move a new BSON value in.
So, neither of these is particularly appealing.
The currently proposed behavior is consistent with the first option (allow it to point to nothing). Despite the misgivings in the quoted discussion, because bsoncxx::v1::document::view significantly improves handling of the "invalid" state, I believe the user experience will not be as poor as prior discussions were concerned might be the case. For bsoncxx::v1::document::value, the null pointer is only observable via .release(). For a resulting bsoncxx::v1::document::view, aside from the results of .data(), .size(), and .empty() (which are all well-defined), the observable behavior is otherwise consistent with the behavior of an empty BSON document.
bsoncxx::v1::document::value doc;
auto view = doc.view();
// Inherited from value.
REQUIRE(view.data() == nullptr);
CHECK(view.size() == 0u);
// Well-defined behavior and no exceptions.
CHECK_FALSE(view.empty());
CHECK_FALSE(view);
CHECK(view.begin() == view.end());
CHECK(view.find("x") == view.end());
CHECK(view == view);By comparison, the current v_noabi interface is very bug-prone:
bsoncxx::document::value doc{nullptr, 0u, nullptr};
auto view = doc.view();
// Inherited from value.
REQUIRE(view.data() == nullptr);
CHECK(view.size() == 0u);
CHECK_FALSE(view.empty());
// CHECK_FALSE(view); // Not supported or well-specified.
CHECK_NOTHROW(view.end());
// Bug-prone behavior: termination(!) due to internal null pointer assertions.
// CHECK_NOTHROW(view.begin());
// CHECK_NOTHROW(view.find("x"));
// CHECK(view == view);| /// When the type of `lhs` and `rhs` compare equal but are not a supported value (not defined by @ref | ||
| /// BSONCXX_V1_TYPES_XMACRO), the result is always `true`. |
There was a problem hiding this comment.
Instead, all v1 interfaces ensure equality comparison has no preconditions and cannot fail.
Confirming: is avoiding an exception intended to avoid preconditions in general? Or is equality treated specially to avoid throwing?
| /// | ||
| /// The number of bytes required to represent an ObjectID. | ||
| /// | ||
| static constexpr BSONCXX_ABI_EXPORT std::size_t k_oid_length = 12; |
There was a problem hiding this comment.
No strong opinion. I am OK with the proposed change (appears consistent with v_noabi), but would not object to switching to an enum constant to avoid adding to the ABI.
@vector-of-bool do you have an opinion?
There was a problem hiding this comment.
Suggestions applied. Open questions following first round of review include:
- Should compile-time constants be defined as enum constants (not exported in the ABI) instead of
static constexprvariables (exported in the ABI due to pre-C++17 compatibility requirements)? ShouldFeedback: no.v1::document::value::view()return an "empty doc" view instead of an invalid view without otherwise changing its allocation behavior (_data.get() == nullptr)?ShouldFeedback: no.v1::document::valuecontinue to support implicit assignment from av1::document::view(currently requiresdoc = value{view};)?
| friend bool operator==(view const& lhs, view const& rhs) { | ||
| if (!lhs != !rhs) { | ||
| return false; | ||
| } | ||
|
|
||
| return !lhs || (lhs.raw() == rhs.raw() && lhs.offset() == rhs.offset()); | ||
| } |
There was a problem hiding this comment.
Makes sense. Reverted v1::element::view to be non-equality-comparable.
The operation is moved back into const_iterator's equality comparison operator.
|
|
||
| public: | ||
| /// | ||
| /// Initialize as an empty view. |
There was a problem hiding this comment.
It is both for compatibility with v_noabi::document::view as well as consistency with std::string_view, which is default-initialized as an "empty string" rather than an "invalid string view".
It follows that for consistency with std::string, a default-constructed v1::document::value should also be an empty document rather than invalid. However, as noted in CXX-939 and related discussions, this complicates the allocation strategy for representing an empty BSON document (uncondition allocation? use pre-allocated storage? etc.).
We could consider leaving the currently-proposed v1::document::value semantics the same, but make the following tweak to the behavior of this->view():
v1::document::view view() const {
if (v1::document::view const v{_data.get(), _length}) {
return v;
}
return {};
}The result of this tweak would be that a default-initialized value is equivalent to a default-initialized view (both an "empty document") even when value._data.get() == nullptr. No unconditional or special-cased allocation is required. Since all non-ownership-related accessors are implemented in terms of this->view(), the null pointer would not be "observable" to the user via normal BSON document operations except via this->release(). However, this PR did not initially propose this due concern that this may be more confusing to users than the current inconsistency between view vs. value default initialization.
Thoughts?
| /// When the type of `lhs` and `rhs` compare equal but are not a supported value (not defined by @ref | ||
| /// BSONCXX_V1_TYPES_XMACRO), the result is always `true`. |
There was a problem hiding this comment.
The latter. Even if they may not be noexcept, it is usually astonishing to the user when the expression a == b can "fail". It is difficult to justify an equality comparison for a regular type having preconditions (that is, possibly UB) or to throw an exception (the postconditions could not be satisfied): that is, why should either be the case? There is an argument for comparison with "unsupported values" to throw an exception, but I believe "unspecified" is a safer and more user-friendly approach.
On that note, took this opportunity to change the documentation of comparing unsupported types from "true" to "unspecified".
| /// | ||
| /// Equivalent to `to_bson(v, *this);` after default-initialization. | ||
| /// | ||
| /// @par Constraints: | ||
| /// - `T` is not @ref bsoncxx::v1::document::value. | ||
| /// - `to_bson(v, *this)` is a valid and unambiguous overload found by ADL. | ||
| /// | ||
| template <typename T, detail::enable_if_t<has_to_bson<T>::value>* = nullptr> | ||
| explicit value(T const& v) : value{} { | ||
| to_bson(v, *this); // ADL. | ||
| } | ||
|
|
||
| /// | ||
| /// Equivalent to `*this = value{v}`. | ||
| /// | ||
| /// @par Constraints: | ||
| /// - `T` is not @ref bsoncxx::v1::document::value. | ||
| /// - `to_bson(v, *this)` is a valid and unambiguous overload found by ADL. | ||
| /// | ||
| template <typename T, detail::enable_if_t<has_to_bson<T>::value>* = nullptr> | ||
| value& operator=(T const& v) { | ||
| *this = value{v}; | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
After further investigation, it seems that default constructibility is possible to support while preserving backward compatibility with v_noabi::document::value by making the default constructor explicit:
explicit value() = default;This ensures that existing patterns of the form v_noabi::document::value doc({}); to workaround lack of default-constructibility continue to work as expected without leading to ambiguity with SMFs: only the view constructor's parameter can be copy-initialized with {}.
The aforementioned issues concerning the lack of constraints was confusing the default constructibility problem with view -> value implicit conversion on assignment, where v_noabi::document::value doc; doc = view; is permitted not by an operator=(view), but via operator=(T const&) [T = view] despite no to_bson overload.
An operator=(view) could be added for consistency with v_noabi, but the current API proposes omitting this overload to ensure consistency of view -> value conversion being explicit.
Thoughts?
| /// | ||
| /// Equivalent to `to_bson(v, *this);` after default-initialization. | ||
| /// | ||
| /// @par Constraints: | ||
| /// - `T` is not @ref bsoncxx::v1::document::value. | ||
| /// - `to_bson(v, *this)` is a valid and unambiguous overload found by ADL. | ||
| /// | ||
| template <typename T, detail::enable_if_t<has_to_bson<T>::value>* = nullptr> | ||
| explicit value(T const& v) : value{} { | ||
| to_bson(v, *this); // ADL. | ||
| } | ||
|
|
||
| /// | ||
| /// Equivalent to `*this = value{v}`. | ||
| /// | ||
| /// @par Constraints: | ||
| /// - `T` is not @ref bsoncxx::v1::document::value. | ||
| /// - `to_bson(v, *this)` is a valid and unambiguous overload found by ADL. | ||
| /// | ||
| template <typename T, detail::enable_if_t<has_to_bson<T>::value>* = nullptr> | ||
| value& operator=(T const& v) { | ||
| *this = value{v}; | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
by making the default constructor explicit
I support this change. I do not see an obvious downside, and it may help ease migrating from v_noabi to v1.
An
operator=(view)could be added for consistency withv_noabi
I slightly prefer the explicitview -> valueconversion to make the data copy explicit.
Resolves CXX-3232. Followup to #1318, #1401, and #1402.
This is 2 out of an estimated 7 major PRs which in total are expected to resolve CXX-2745.
This PR introduces the very first set of v1 ABI exported symbols. 🎉
... only because MSVC complains otherwise. The rest will come as planned in CXX-3233 (impls).
Due to the volume of changes relative to the equivalent v_noabi interfaces, the bulk of relative changelog (v_noabi -> v1) and rationale for design decisions are documented inline as GitHub comments (coincidentally, exactly 100 comments in total).
In summary, notable changes include:
v1::errorv1::errorwith corresponding error category declared inv1::error::category.sourceandtypeare provided as the primary user-facing error code query mechanism when component-specific error codes are not necessary.v1::types::b_*value).v1::types::viewvaluewhen applicable.v1::types::valueb_*is always favored) so eitherb_*orview(cheap) is always preferred tovalue(expensive) when applicable.v1::types::idconstructors are removed in favor ofb_*and converting constructors.v1::element::viewbool-based approach to "missing field" diagnostics (CXX-2120).v1::document::viewv1::array::viewv1::document::view.v1::document::view(CXX-2118).v1::document::valuev1::document::view.deleter_typeis now astd::function<T>instead of a function pointer.to_bsonandfrom_bsonare now properly constrained.v1::array::valuev1::document::value.v1::document::value(CXX-2118).