From a58b4870d70e1d7450a7001624a81bf56010b552 Mon Sep 17 00:00:00 2001 From: Dennis Klein Date: Fri, 24 Feb 2023 11:26:20 +0100 Subject: [PATCH] feat(Parts): Refine and tweak * Optimize appending another Parts container * Remove redundant/verbose comments * Change r-value args to move-only types into l-value args for readability * Deprecate `AtRef(int)`, redundant, just dereference at call site * Deprecate `AddPart(Message*)`, avoid owning raw pointer args * Add various const overloads * Add `Empty()` and `Clear()` member functions * Add `noexcept` where applicable --- fairmq/Parts.h | 98 +++++++++++++++++++++------------------- test/CMakeLists.txt | 3 +- test/parts/_add_part.cxx | 73 ++++++++++++++++++++++++++++++ 3 files changed, 127 insertions(+), 47 deletions(-) create mode 100644 test/parts/_add_part.cxx diff --git a/fairmq/Parts.h b/fairmq/Parts.h index a8f5811f..4c0cbc39 100644 --- a/fairmq/Parts.h +++ b/fairmq/Parts.h @@ -1,5 +1,5 @@ /******************************************************************************** - * Copyright (C) 2014-2022 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH * + * Copyright (C) 2014-2023 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH * * * * This software is distributed under the terms of the * * GNU Lesser General Public Licence (LGPL) version 3, * @@ -9,79 +9,85 @@ #ifndef FAIR_MQ_PARTS_H #define FAIR_MQ_PARTS_H -#include -#include // unique_ptr -#include +#include // std::move +#include // fair::mq::MessagePtr +#include // std::back_inserter +#include // std::move, std::forward +#include // std::vector namespace fair::mq { -/// fair::mq::Parts is a lightweight convenience wrapper around a vector of unique pointers to +/// fair::mq::Parts is a lightweight move-only convenience wrapper around a vector of unique pointers to /// Message, used for sending multi-part messages -class Parts +struct Parts { - private: using container = std::vector; + using size_type = container::size_type; + using reference = container::reference; + using const_reference = container::const_reference; + using iterator = container::iterator; + using const_iterator = container::const_iterator; - public: - Parts() = default; + Parts() noexcept(noexcept(container())) = default; Parts(const Parts&) = delete; - Parts(Parts&&) = default; - template - Parts(Ts&&... messages) { AddPart(std::forward(messages)...); } Parts& operator=(const Parts&) = delete; + Parts(Parts&&) = default; Parts& operator=(Parts&&) = default; ~Parts() = default; - /// Adds part (Message) to the container - /// @param msg message pointer (for example created with NewMessage() method of Device) + template + Parts(Ps&&... parts) + { + fParts.reserve(sizeof...(Ps)); + AddPart(std::forward(parts)...); + } + + [[deprecated("Avoid owning raw pointer args, use AddPart(MessagePtr) instead.")]] void AddPart(Message* msg) { fParts.push_back(MessagePtr(msg)); } + void AddPart(MessagePtr msg) { fParts.push_back(std::move(msg)); } - /// Adds part to the container (move) - /// @param msg unique pointer to Message - /// rvalue ref (move required when passing argument) - void AddPart(MessagePtr&& msg) { fParts.push_back(std::move(msg)); } - - /// Add variable list of parts to the container (move) template - void AddPart(MessagePtr&& first, Ts&&... remaining) + void AddPart(MessagePtr first, Ts&&... remaining) { AddPart(std::move(first)); AddPart(std::forward(remaining)...); } - /// Add content of another object by move - void AddPart(Parts&& other) + void AddPart(Parts parts) { - container parts = std::move(other.fParts); - for (auto& part : parts) { - fParts.push_back(std::move(part)); + if (fParts.empty()) { + fParts = std::move(parts.fParts); + } else { + fParts.reserve(parts.Size() + fParts.size()); + std::move(std::begin(parts), std::end(parts), std::back_inserter(fParts)); } } - /// Get reference to part in the container at index (without bounds check) - /// @param index container index - Message& operator[](const int index) { return *(fParts[index]); } + Message& operator[](size_type index) { return *(fParts[index]); } + Message const& operator[](size_type index) const { return *(fParts[index]); } + // TODO: For consistency with the STL interfaces, operator[] should not dereference, + // but I have no good idea how to fix this. + // reference operator[](size_type index) { return fParts[index]; } + // const_reference operator[](size_type index) const { return fParts[index]; } - /// Get reference to unique pointer to part in the container at index (with bounds check) - /// @param index container index - MessagePtr& At(const int index) { return fParts.at(index); } + [[deprecated("Redundant, dereference at call site e.g. '*(parts.At(index))' instead.")]] + Message& AtRef(size_type index) { return *(fParts.at(index)); } + reference At(size_type index) { return fParts.at(index); } + const_reference At(size_type index) const { return fParts.at(index); } - // ref version - Message& AtRef(const int index) { return *(fParts.at(index)); } + size_type Size() const noexcept { return fParts.size(); } + bool Empty() const noexcept { return fParts.empty(); } + void Clear() noexcept { fParts.clear(); } - /// Get number of parts in the container - /// @return number of parts in the container - int Size() const { return fParts.size(); } + // range access + iterator begin() noexcept { return fParts.begin(); } + const_iterator begin() const noexcept { return fParts.begin(); } + const_iterator cbegin() const noexcept { return fParts.cbegin(); } + iterator end() noexcept { return fParts.end(); } + const_iterator end() const noexcept { return fParts.end(); } + const_iterator cend() const noexcept { return fParts.cend(); } - container fParts; - - // forward container iterators - using iterator = container::iterator; - using const_iterator = container::const_iterator; - auto begin() -> decltype(fParts.begin()) { return fParts.begin(); } - auto end() -> decltype(fParts.end()) { return fParts.end(); } - auto cbegin() -> decltype(fParts.cbegin()) { return fParts.cbegin(); } - auto cend() -> decltype(fParts.cend()) { return fParts.cend(); } + container fParts{}; }; } // namespace fair::mq diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index b4527ab2..56953234 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -1,5 +1,5 @@ ################################################################################ -# Copyright (C) 2014-2022 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH # +# Copyright (C) 2014-2023 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH # # # # This software is distributed under the terms of the # # GNU Lesser General Public Licence (LGPL) version 3, # @@ -72,6 +72,7 @@ add_testsuite(Protocols add_testsuite(Parts SOURCES ${CMAKE_CURRENT_BINARY_DIR}/runner.cxx + parts/_add_part.cxx parts/_iterator_interface.cxx LINKS FairMQ diff --git a/test/parts/_add_part.cxx b/test/parts/_add_part.cxx new file mode 100644 index 00000000..9cb723e4 --- /dev/null +++ b/test/parts/_add_part.cxx @@ -0,0 +1,73 @@ +/******************************************************************************** + * Copyright (C) 2023 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH * + * * + * This software is distributed under the terms of the * + * GNU Lesser General Public Licence (LGPL) version 3, * + * copied verbatim in the file "LICENSE" * + ********************************************************************************/ + +#include +#include +#include +#include + +namespace { + +using namespace std; + +class AddPart : public ::testing::Test +{ + protected: + fair::mq::Parts mParts; + shared_ptr mFactory; + + AddPart() + : mFactory(fair::mq::TransportFactory::CreateTransportFactory("zeromq")) + { + } +}; + +TEST_F(AddPart, AnotherParts) +{ + mParts.AddPart(mFactory->NewSimpleMessage("1")); + mParts.AddPart(mFactory->NewSimpleMessage("2")); + mParts.AddPart(mFactory->NewSimpleMessage("3")); + auto const oldSize = mParts.Size(); + + fair::mq::Parts anotherParts; + anotherParts.AddPart(mFactory->NewSimpleMessage("4")); + anotherParts.AddPart(mFactory->NewSimpleMessage("5")); + anotherParts.AddPart(mFactory->NewSimpleMessage("6")); + auto const addedSize = anotherParts.Size(); + auto const newSize = oldSize + addedSize; + + mParts.AddPart(std::move(anotherParts)); + ASSERT_TRUE(mParts.Size() == newSize); +} + +TEST_F(AddPart, SinglePart) +{ + auto const oldSize = mParts.Size(); + mParts.AddPart(mFactory->NewSimpleMessage("asdf")); + ASSERT_TRUE(mParts.Size() == oldSize + 1); +} + +TEST_F(AddPart, MultipleParts) +{ + auto const oldSize = mParts.Size(); + mParts.AddPart(mFactory->NewSimpleMessage("1"), + mFactory->NewSimpleMessage("2"), + mFactory->NewSimpleMessage("3")); + ASSERT_TRUE(mParts.Size() == oldSize + 3); +} + +TEST(Construction, AppendMultipleParts) +{ + auto factory = fair::mq::TransportFactory::CreateTransportFactory("zeromq"); + fair::mq::Parts newParts(factory->NewSimpleMessage("1"), + factory->NewSimpleMessage("2"), + factory->NewSimpleMessage("3")); + ASSERT_TRUE(newParts.Size() == 3); +} + +} // namespace