From: Philippe Proulx Date: Sat, 14 May 2022 16:28:52 +0000 (-0400) Subject: Fix: bt2::internal::SharedObj: add real copy/move ctor./operator=() X-Git-Url: http://git.efficios.com/?a=commitdiff_plain;h=e60a53f18299b25741db35815997690f02f9a0f7;p=babeltrace.git Fix: bt2::internal::SharedObj: add real copy/move ctor./operator=() Observed issue ============== Given the type S, some `bt2::internal::SharedObj` class template instantiation: * Constructing an S object A from another S object doesn't increment the internal reference count of the libbabeltrace2 object which A wraps. * Assigning an S object A from an S object B: * Doesn't increment the internal reference count of the libbabeltrace2 object which B wraps. * Doesn't decrement the internal reference count of the libbabeltrace2 object which A wraps. * Constructing an S object from another moved S object B doesn't reset B. * Assigning an S object A from a moved S object B: * Doesn't reset B. * Doesn't decrement the internal reference count of the libbabeltrace2 object which A wraps. Cause ===== What I thought were copy/move constructor and assignment operators in `bt2::internal::SharedObj` are in fact simple constructor and assignment operator templates. In other words, they don't qualify as copy/move constructor and assignment operators, so the compiler uses default ones. From [1]: > A copy constructor of class `T` is a non-template constructor whose > first parameter is `T&‍`, `const T&‍`, `volatile T&‍`, or `const > volatile T&‍`, and either there are no other parameters, or the rest > of the parameters all have default values. The same condition (non-template) holds for a move constructor, a copy assignment operator, and a move assignment operator. Solution ======== Add copy/move constructors and assignment operators to `bt2::internal::SharedObj`. Although you may call an operator=() method template like this: this->operator=(other) you can't specify the template parameters when you call a constructor template: the types of all its parameters need to be deduced. This is why, to avoid redundant code, I added two common "copy" and "move" (not real copy/move constructors) private constructor templates to which the true copy/move constructors and the generic "copy"/"move" constructors delegate. Those common ones accept a second, unused `int` parameter to differentiate them from the public, generic "copy"/"move" constructors. Known drawbacks =============== None. References ========== [1]: https://en.cppreference.com/w/cpp/language/copy_constructor Change-Id: I6d8cb7c16da0a79296a482f8d130ab40468cc1a5 Reviewed-on: https://review.lttng.org/c/babeltrace/+/8041 Reviewed-by: Philippe Proulx Reviewed-by: Francis Deslauriers Reviewed-on: https://review.lttng.org/c/babeltrace/+/10798 Tested-by: jenkins CI-Build: Philippe Proulx --- diff --git a/src/cpp-common/bt2/internal/shared-obj.hpp b/src/cpp-common/bt2/internal/shared-obj.hpp index d16eb940..da8e42f7 100644 --- a/src/cpp-common/bt2/internal/shared-obj.hpp +++ b/src/cpp-common/bt2/internal/shared-obj.hpp @@ -39,7 +39,7 @@ class SharedObj final * `SharedObj` instance to get * assigned an instance of * `SharedObj` (copy/move - * constructor and assignment operator), given that + * constructors and assignment operators), given that * `SpecificSomething` inherits `Something`. */ template @@ -53,6 +53,36 @@ private: { } + /* + * Common generic "copy" constructor. + * + * This constructor is meant to be delegated to by the copy + * constructor and the generic "copy" constructor. + * + * The second parameter, of type `int`, makes it possible to + * delegate by deduction as you can't explicit the template + * parameters when delegating to a constructor template. + */ + template + SharedObj(const SharedObj& other, int) noexcept : + _mObj {other._mObj} + { + this->_getRef(); + } + + /* + * Common generic "move" constructor. + * + * See the comment of the common generic "copy" constructor above. + */ + template + SharedObj(SharedObj&& other, int) noexcept : + _mObj {other._mObj} + { + /* Reset moved-from object */ + other._reset(); + } + public: /* * Builds a shared object from `obj` without getting a reference. @@ -93,31 +123,61 @@ public: } /* - * Generic copy constructor. + * Copy constructor. + */ + SharedObj(const SharedObj& other) noexcept : SharedObj {other, 0} + { + } + + /* + * Move constructor. + */ + SharedObj(SharedObj&& other) noexcept : SharedObj {std::move(other), 0} + { + } + + /* + * Copy assignment operator. + */ + SharedObj& operator=(const SharedObj& other) noexcept + { + /* Use generic "copy" assignment operator */ + return this->operator=(other); + } + + /* + * Move assignment operator. + */ + SharedObj& operator=(SharedObj&& other) noexcept + { + /* Use generic "move" assignment operator */ + return this->operator=(std::move(other)); + } + + /* + * Generic "copy" constructor. * * See the `friend class SharedObj` comment above. */ template SharedObj(const SharedObj& other) noexcept : - _mObj {other._mObj} + SharedObj {other, 0} { - this->_getRef(); } /* - * Generic move constructor. + * Generic "move" constructor. * * See the `friend class SharedObj` comment above. */ template - SharedObj(SharedObj&& other) noexcept : _mObj {other._mObj} + SharedObj(SharedObj&& other) noexcept : + SharedObj {std::move(other), 0} { - /* Reset moved-from object */ - other._reset(); } /* - * Generic copy assignment operator. + * Generic "copy" assignment operator. * * See the `friend class SharedObj` comment above. */ @@ -135,7 +195,7 @@ public: } /* - * Generic move assignment operator. + * Generic "move" assignment operator. * * See the `friend class SharedObj` comment above. */