Skip to content

Commit

Permalink
incorporate review suggestions
Browse files Browse the repository at this point in the history
  • Loading branch information
@mschimek
mschimek committed Sep 12, 2024
1 parent b2ae891 commit 906f237
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 41 deletions.
71 changes: 35 additions & 36 deletions include/kamping/data_buffer.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,8 @@ namespace kamping {

namespace internal {

/// @brief Base class containing logic to eo ensure whether a buffers data has already been extracted. This only has
/// effects if an appropiate assertion level is set.
class ExtractionStatusBase {
/// @brief Base class containing logic to verify whether a buffer's data has already been extracted. This only has effects if an appropiate assertion level is set.
class Extractable {
protected:
/// @brief Set the extracted flag to indicate that the status stored in this buffer has been moved out.
void set_extracted() {
Expand Down Expand Up @@ -82,37 +81,37 @@ class ExtractionStatusBase {
/// can not be default constructed, due to the missing implementation in the base class. Because we provide a (default)
/// implementation for the move constructor (assignment) in the base class, the derived class can construct default
/// implementations.
template <bool /*has_copy_constructor*/ = false>
class ParameterObjectBase : protected ExtractionStatusBase {
template <bool /*enable_copy_constructor*/ = false>
class CopyMoveEnabler {
protected:
constexpr ParameterObjectBase() = default;
~ParameterObjectBase() = default;
constexpr CopyMoveEnabler() = default;
~CopyMoveEnabler() = default;

/// @brief Copy constructor is deleted as buffers should only be moved.
ParameterObjectBase(ParameterObjectBase const&) = delete;
CopyMoveEnabler(CopyMoveEnabler const&) = delete;
/// @brief Copy assignment operator is deleted as buffers should only be moved.
ParameterObjectBase& operator=(ParameterObjectBase const&) = delete;
CopyMoveEnabler& operator=(CopyMoveEnabler const&) = delete;
/// @brief Move constructor.
ParameterObjectBase(ParameterObjectBase&&) = default;
CopyMoveEnabler(CopyMoveEnabler&&) = default;
/// @brief Move assignment operator.
ParameterObjectBase& operator=(ParameterObjectBase&&) = default;
CopyMoveEnabler& operator=(CopyMoveEnabler&&) = default;
};

/// @brief Specialisation of ParameterObjectBase which possesses a copy constructor.
template <>
class ParameterObjectBase<true> : protected ExtractionStatusBase {
class CopyMoveEnabler<true> {
protected:
constexpr ParameterObjectBase() = default;
~ParameterObjectBase() = default;
constexpr CopyMoveEnabler() = default;
~CopyMoveEnabler() = default;

/// @brief Copy constructor is enabled (this is okay for buffers which only reference their data)
ParameterObjectBase(ParameterObjectBase const&) = default;
CopyMoveEnabler(CopyMoveEnabler const&) = default;
/// @brief Copy assignment operator is deleted as buffers should only be moved.
ParameterObjectBase& operator=(ParameterObjectBase const&) = delete;
CopyMoveEnabler& operator=(CopyMoveEnabler const&) = delete;
/// @brief Move constructor.
ParameterObjectBase(ParameterObjectBase&&) = default;
CopyMoveEnabler(CopyMoveEnabler&&) = default;
/// @brief Move assignment operator.
ParameterObjectBase& operator=(ParameterObjectBase&&) = default;
CopyMoveEnabler& operator=(CopyMoveEnabler&&) = default;
};

/// @brief Boolean value helping to decide if type has a \c value_type member type.
Expand Down Expand Up @@ -369,7 +368,7 @@ template <
BufferResizePolicy buffer_resize_policy_param,
BufferAllocation allocation = BufferAllocation::user_allocated,
typename ValueType = default_value_type_tag>
class DataBuffer : private ParameterObjectBase<enable_copy_construction<ownership>> {
class DataBuffer : private CopyMoveEnabler<enable_copy_construction<ownership>>, private Extractable {
public:
static constexpr TParameterType parameter_type =
parameter_type_param; ///< The type of parameter this buffer represents.
Expand Down Expand Up @@ -455,7 +454,7 @@ class DataBuffer : private ParameterObjectBase<enable_copy_construction<ownershi

/// @brief The size of the underlying container.
size_t size() const {
this->kassert_not_extracted("Cannot get the size of a buffer that has already been extracted.");
kassert_not_extracted("Cannot get the size of a buffer that has already been extracted.");
if constexpr (is_single_element) {
return 1;
} else {
Expand All @@ -474,7 +473,7 @@ class DataBuffer : private ParameterObjectBase<enable_copy_construction<ownershi
BufferResizePolicy _resize_policy = resize_policy,
typename std::enable_if_t<_resize_policy == resize_to_fit, bool> = true>
void resize(size_t size) {
this->kassert_not_extracted("Cannot resize a buffer that has already been extracted.");
kassert_not_extracted("Cannot resize a buffer that has already been extracted.");
underlying().resize(size);
}

Expand All @@ -489,7 +488,7 @@ class DataBuffer : private ParameterObjectBase<enable_copy_construction<ownershi
BufferResizePolicy _resize_policy = resize_policy,
typename std::enable_if_t<_resize_policy == grow_only, bool> = true>
void resize(size_t size) {
this->kassert_not_extracted("Cannot resize a buffer that has already been extracted.");
kassert_not_extracted("Cannot resize a buffer that has already been extracted.");
if (this->size() < size) {
underlying().resize(size);
}
Expand All @@ -516,7 +515,7 @@ class DataBuffer : private ParameterObjectBase<enable_copy_construction<ownershi
/// @brief Get const access to the underlying container.
/// @return Pointer to the underlying container.
value_type const* data() const {
this->kassert_not_extracted("Cannot get a pointer to a buffer that has already been extracted.");
kassert_not_extracted("Cannot get a pointer to a buffer that has already been extracted.");
if constexpr (is_single_element) {
return &underlying();
} else {
Expand All @@ -527,7 +526,7 @@ class DataBuffer : private ParameterObjectBase<enable_copy_construction<ownershi
/// @brief Get access to the underlying container.
/// @return Pointer to the underlying container.
value_type_with_const* data() {
this->kassert_not_extracted("Cannot get a pointer to a buffer that has already been extracted.");
kassert_not_extracted("Cannot get a pointer to a buffer that has already been extracted.");
if constexpr (is_single_element) {
return &underlying();
} else {
Expand All @@ -538,29 +537,29 @@ class DataBuffer : private ParameterObjectBase<enable_copy_construction<ownershi
/// @brief Get read-only access to the underlying storage.
/// @return Span referring the underlying storage.
Span<value_type const> get() const {
this->kassert_not_extracted("Cannot get a buffer that has already been extracted.");
kassert_not_extracted("Cannot get a buffer that has already been extracted.");
return {this->data(), this->size()};
}

/// @brief Get access to the underlying storage.
/// @return Span referring to the underlying storage.
Span<value_type_with_const> get() {
this->kassert_not_extracted("Cannot get a buffer that has already been extracted.");
kassert_not_extracted("Cannot get a buffer that has already been extracted.");
return {this->data(), this->size()};
}

/// @brief Get the single element wrapped by this object.
/// @return The single element wrapped by this object.
template <bool enabled = is_single_element, std::enable_if_t<enabled, bool> = true>
value_type const get_single_element() const {
this->kassert_not_extracted("Cannot get an element from a buffer that has already been extracted.");
kassert_not_extracted("Cannot get an element from a buffer that has already been extracted.");
return underlying();
}

/// @brief Provides access to the underlying data.
/// @return A reference to the data.
MemberType const& underlying() const {
this->kassert_not_extracted("Cannot get a buffer that has already been extracted.");
kassert_not_extracted("Cannot get a buffer that has already been extracted.");
// this assertion is only checked if the buffer is actually accessed.
static_assert(
!is_vector_bool_v<MemberType>,
Expand All @@ -573,7 +572,7 @@ class DataBuffer : private ParameterObjectBase<enable_copy_construction<ownershi
/// @return A reference to the data.
template <bool enabled = modifiability == BufferModifiability::modifiable, std::enable_if_t<enabled, bool> = true>
MemberType& underlying() {
this->kassert_not_extracted("Cannot get a buffer that has already been extracted.");
kassert_not_extracted("Cannot get a buffer that has already been extracted.");
// this assertion is only checked if the buffer is actually accessed.
static_assert(
!is_vector_bool_v<MemberType>,
Expand All @@ -593,10 +592,10 @@ class DataBuffer : private ParameterObjectBase<enable_copy_construction<ownershi
"Moving out of a reference should not be done because it would leave "
"a users container in an unspecified state."
);
this->kassert_not_extracted("Cannot extract a buffer that has already been extracted.");
kassert_not_extracted("Cannot extract a buffer that has already been extracted.");
auto extracted = std::move(underlying());
// we set is_extracted here because otherwise the call to underlying() would fail
this->set_extracted();
set_extracted();
return extracted;
}

Expand All @@ -623,7 +622,7 @@ template <
BufferModifiability modifiability,
BufferOwnership ownership,
BufferType buffer_type_param>
class GenericDataBuffer : private ParameterObjectBase<enable_copy_construction<ownership>> {
class GenericDataBuffer : private CopyMoveEnabler<enable_copy_construction<ownership>>, private Extractable {
public:
static constexpr TParameterType parameter_type =
parameter_type_param; ///< The type of parameter this buffer represents.
Expand Down Expand Up @@ -667,15 +666,15 @@ class GenericDataBuffer : private ParameterObjectBase<enable_copy_construction<o
/// @brief Provides access to the underlying data.
/// @return A reference to the data.
MemberType const& underlying() const {
this->kassert_not_extracted("Cannot get a buffer that has already been extracted.");
kassert_not_extracted("Cannot get a buffer that has already been extracted.");
return _data;
}

/// @brief Provides access to the underlying data.
/// @return A reference to the data.
template <bool enabled = modifiability == BufferModifiability::modifiable, std::enable_if_t<enabled, bool> = true>
MemberType& underlying() {
this->kassert_not_extracted("Cannot get a buffer that has already been extracted.");
kassert_not_extracted("Cannot get a buffer that has already been extracted.");
return _data;
}

Expand All @@ -690,10 +689,10 @@ class GenericDataBuffer : private ParameterObjectBase<enable_copy_construction<o
"Moving out of a reference should not be done because it would leave "
"a users container in an unspecified state."
);
this->kassert_not_extracted("Cannot extract a buffer that has already been extracted.");
kassert_not_extracted("Cannot extract a buffer that has already been extracted.");
auto extracted = std::move(underlying());
// we set is_extracted here because otherwise the call to underlying() would fail
this->set_extracted();
set_extracted();
return extracted;
}

Expand Down
10 changes: 5 additions & 5 deletions include/kamping/parameter_objects.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,7 @@ class RankDataBuffer<RankType::value, type> final : private DataBuffer<
/// This is a specialization for MPI_ANY_SOURCE which only implements
/// \ref rank_signed(), without allocating any additional memory.
template <ParameterType type>
class RankDataBuffer<RankType::any, type> : private ParameterObjectBase<> {
class RankDataBuffer<RankType::any, type> : private CopyMoveEnabler<> {
public:
static constexpr ParameterType parameter_type = type; ///< The type of parameter this object encapsulates.
static constexpr RankType rank_type = RankType::any; ///< The rank type.
Expand All @@ -444,7 +444,7 @@ class RankDataBuffer<RankType::any, type> : private ParameterObjectBase<> {
/// This is a specialization for MPI_PROC_NULL which only implements
/// \ref rank_signed(), without allocating any additional memory.
template <ParameterType type>
class RankDataBuffer<RankType::null, type> : private ParameterObjectBase<> {
class RankDataBuffer<RankType::null, type> : private CopyMoveEnabler<> {
public:
static constexpr ParameterType parameter_type = type; ///< The type of parameter this object encapsulates.
static constexpr RankType rank_type = RankType::null; ///< The rank type.
Expand Down Expand Up @@ -475,7 +475,7 @@ using send_mode_list =
/// @brief Parameter object for send_mode encapsulating the send mode compile-time tag.
/// @tparam SendModeTag The send mode.
template <typename SendModeTag>
struct SendModeParameter : private ParameterObjectBase<> {
struct SendModeParameter : private CopyMoveEnabler<> {
static_assert(send_mode_list::contains<SendModeTag>, "Unsupported send mode.");
static constexpr ParameterType parameter_type = ParameterType::send_mode; ///< The parameter type.
using send_mode = SendModeTag; ///< The send mode.
Expand Down Expand Up @@ -514,7 +514,7 @@ class TagParam {};

/// @brief Encapsulates a message tag. Specialization if an explicit tag value is provided.
template <>
class TagParam<TagType::value> : private ParameterObjectBase<> {
class TagParam<TagType::value> : private CopyMoveEnabler<> {
public:
/// @param tag The tag.
TagParam(int tag) : _tag_value(tag) {}
Expand All @@ -538,7 +538,7 @@ class TagParam<TagType::value> : private ParameterObjectBase<> {

/// @brief Encapsulates a message tag. Specialization if the value is MPI_ANY_TAG.
template <>
class TagParam<TagType::any> : private ParameterObjectBase<> {
class TagParam<TagType::any> : private CopyMoveEnabler<> {
public:
static constexpr ParameterType parameter_type = ParameterType::tag; ///< The parameter type.
static constexpr TagType tag_type = TagType::any; ///< The tag type.
Expand Down

0 comments on commit 906f237

Please sign in to comment.