Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new mutex+data wrapper (ported from clio) intended to bundle protected data with its mutex and provide an RAII lock object that grants scoped access to the data.
Changes:
- Add
xrpl::Mutex<T, MutexType>container that ownsTand a mutex. - Add
xrpl::Lock<T, LockType, MutexType>to hold the lock and provide*,->, andget()accessors.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * @brief Make a new Mutex object with the given data | ||
| * | ||
| * @tparam Args The types of the arguments to forward to the constructor of the protected data | ||
| * @param args The arguments to forward to the constructor of the protected data | ||
| * @return The Mutex object that protects the given data | ||
| */ | ||
| template <typename... Args> | ||
| static Mutex | ||
| make(Args&&... args) | ||
| { | ||
| return Mutex{ProtectedDataType{std::forward<Args>(args)...}}; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Lock the mutex and get a lock object allowing access to the protected data | ||
| * | ||
| * @tparam LockType The type of lock to use | ||
| * @return A lock on the mutex and a reference to the protected data | ||
| */ | ||
| template <template <typename...> typename LockType = std::lock_guard> | ||
| Lock<ProtectedDataType const, LockType, MutexType> | ||
| lock() const | ||
| { | ||
| return {mutex_, data_}; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Lock the mutex and get a lock object allowing access to the protected data | ||
| * | ||
| * @tparam LockType The type of lock to use | ||
| * @return A lock on the mutex and a reference to the protected data | ||
| */ | ||
| template <template <typename...> typename LockType = std::lock_guard> | ||
| Lock<ProtectedDataType, LockType, MutexType> | ||
| lock() | ||
| { | ||
| return {mutex_, data_}; | ||
| } |
There was a problem hiding this comment.
This introduces a new concurrency utility but there are no accompanying unit tests. Since the basics module already has gtest coverage for similar utilities, consider adding a small test that exercises make(...), lock() (const/non-const), and access operators to catch integration/const-correctness regressions.
There was a problem hiding this comment.
I agree with Copilot. Let's add basic checks (no need to test the standard mutex itself):
- Mutex::make(...)
- non-const lock() and mutable access through *, ->, and get()
- const lock() and confirming it exposes const access
- the templated lock() path / conversion to the underlying lock type
// src/tests/libxrpl/basics/Mutex_test.cpp
#include <xrpl/basics/Mutex.hpp>
#include <gtest/gtest.h>
#include <string>
#include <type_traits>
namespace {
struct Widget
{
std::string name;
int value;
Widget(std::string n, int v) : name(n), value(v) {}
void
bump()
{
++value;
}
};
TEST(MutexTest, MakeAndMutableAccess)
{
auto m = xrpl::Mutex<Widget>::make("alice", 7);
{
auto lock = m.lock();
static_assert(std::is_same_v<decltype(*lock), Widget&>);
static_assert(std::is_same_v<decltype(lock.get()), Widget&>);
static_assert(std::is_same_v<decltype(lock.operator->()), Widget*>);
EXPECT_EQ(lock->name, "alice");
EXPECT_EQ((*lock).value, 7);
lock.get().value = 8;
lock->bump();
EXPECT_EQ(lock->value, 9);
}
auto verify = m.lock();
EXPECT_EQ(verify->value, 9);
}
TEST(MutexTest, ConstLockReturnsConstView)
{
xrpl::Mutex<Widget> const m{Widget{"bob", 3}};
auto lock = m.lock();
static_assert(std::is_same_v<decltype(*lock), Widget const&>);
static_assert(std::is_same_v<decltype(lock.get()), Widget const&>);
static_assert(std::is_same_v<decltype(lock.operator->()), Widget const*>);
EXPECT_EQ(lock->name, "bob");
EXPECT_EQ(lock->value, 3);
}
TEST(MutexTest, SupportsAlternateLockType)
{
xrpl::Mutex<Widget> m{Widget{"carol", 0}};
{
auto lock = m.lock<std::unique_lock>();
std::unique_lock<std::mutex>& underlying = lock;
EXPECT_TRUE(underlying.owns_lock());
lock->bump();
EXPECT_EQ(lock->value, 1);
}
}
} // namespace
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6447 +/- ##
=========================================
- Coverage 79.8% 79.8% -0.1%
=========================================
Files 848 848
Lines 67757 67757
Branches 7558 7579 +21
=========================================
- Hits 54074 54039 -35
- Misses 13683 13718 +35 see 9 files with indirect coverage changes 🚀 New features to boost your workflow:
|
vlntb
left a comment
vlntb
left a comment
There was a problem hiding this comment.
LGTM. Let's resolve the question about the unit test, and I'm happy to approve.
High Level Overview of Change
This PR adds a mutex wrapper copied from clio. This wrapper makes a mutex attached to the data it protects which improves safety and readability.
Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)