Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix dynamic connectable #686

Merged
merged 2 commits into from
Nov 21, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions src/rpp/rpp/observables/connectable_observable.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,21 +69,21 @@ namespace rpp
* @ingroup observables
*/
template<rpp::constraint::observable OriginalObservable, typename Subject>
class connectable_observable final : public decltype(std::declval<Subject>().get_observable())
class connectable_observable : public decltype(std::declval<Subject>().get_observable())
{
using base = decltype(std::declval<Subject>().get_observable());

public:
static_assert(rpp::constraint::subject<Subject>);

connectable_observable(const OriginalObservable& original_observable, const Subject& subject = Subject{})
connectable_observable(const OriginalObservable& original_observable, const Subject& subject)
: base{subject.get_observable()}
, m_original_observable{original_observable}
, m_subject{subject}
{
}

connectable_observable(OriginalObservable && original_observable, const Subject& subject = Subject{})
connectable_observable(OriginalObservable && original_observable, const Subject& subject)
: base{subject.get_observable()}
, m_original_observable{std::move(original_observable)}
, m_subject{subject}
Expand Down Expand Up @@ -166,6 +166,15 @@ namespace rpp
return std::move(*this) | std::forward<Op>(op);
}

auto as_dynamic_connectable() const &
{
return rpp::dynamic_connectable_observable<Subject>{m_original_observable.as_dynamic(), m_subject};
}
auto as_dynamic_connectable()&&
{
return rpp::dynamic_connectable_observable<Subject>{std::move(m_original_observable).as_dynamic(), std::move(m_subject)};
}

private:
RPP_NO_UNIQUE_ADDRESS OriginalObservable m_original_observable;
Subject m_subject;
Expand Down
41 changes: 41 additions & 0 deletions src/rpp/rpp/observables/dynamic_connectable_observable.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// ReactivePlusPlus library
//
// Copyright Aleksey Loginov 2023 - present.
// Distributed under the Boost Software License, Version 1.0.
// (See accompanying file LICENSE_1_0.txt or copy at
// https://www.boost.org/LICENSE_1_0.txt)
//
// Project home: https://github.com/victimsnino/ReactivePlusPlus

#pragma once

#include <rpp/observables/connectable_observable.hpp>
#include <rpp/observables/dynamic_observable.hpp>

namespace rpp
{
template<typename Subject>
class dynamic_connectable_observable final : public connectable_observable<rpp::dynamic_observable<rpp::subjects::utils::extract_subject_type_t<Subject>>, Subject>
{
public:
static_assert(rpp::constraint::subject<Subject>);

using base = connectable_observable<rpp::dynamic_observable<rpp::subjects::utils::extract_subject_type_t<Subject>>, Subject>;

using base::base;

template<rpp::constraint::observable_strategy<rpp::subjects::utils::extract_subject_type_t<Subject>> Strategy>
requires (!rpp::constraint::decayed_same_as<Strategy, rpp::dynamic_observable<rpp::subjects::utils::extract_subject_type_t<Subject>>>)
dynamic_connectable_observable(const rpp::connectable_observable<Strategy, Subject>& original)
: dynamic_connectable_observable{original.as_dynamic_connectable()}
{
}

template<rpp::constraint::observable_strategy<rpp::subjects::utils::extract_subject_type_t<Subject>> Strategy>
requires (!rpp::constraint::decayed_same_as<Strategy, rpp::dynamic_observable<rpp::subjects::utils::extract_subject_type_t<Subject>>>)
dynamic_connectable_observable(rpp::connectable_observable<Strategy, Subject>&& original)
: dynamic_connectable_observable{std::move(original).as_dynamic_connectable()}
{
}
};
Comment on lines +27 to +40
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor constructors to eliminate code duplication

The two constructors have similar logic and can be combined to reduce duplication. Consider using a single templated constructor with perfect forwarding.

Apply this diff to refactor the constructors:

-        template<rpp::constraint::observable_strategy<rpp::subjects::utils::extract_subject_type_t<Subject>> Strategy>
-            requires (!rpp::constraint::decayed_same_as<Strategy, rpp::dynamic_observable<rpp::subjects::utils::extract_subject_type_t<Subject>>>)
-        dynamic_connectable_observable(const rpp::connectable_observable<Strategy, Subject>& original)
-            : dynamic_connectable_observable{original.as_dynamic_connectable()}
-        {
-        }
-
-        template<rpp::constraint::observable_strategy<rpp::subjects::utils::extract_subject_type_t<Subject>> Strategy>
-            requires (!rpp::constraint::decayed_same_as<Strategy, rpp::dynamic_observable<rpp::subjects::utils::extract_subject_type_t<Subject>>>)
-        dynamic_connectable_observable(rpp::connectable_observable<Strategy, Subject>&& original)
-            : dynamic_connectable_observable{std::move(original).as_dynamic_connectable()}
-        {
-        }

+        template<
+            rpp::constraint::observable_strategy<rpp::subjects::utils::extract_subject_type_t<Subject>> Strategy
+        >
+            requires (!rpp::constraint::decayed_same_as<Strategy, rpp::dynamic_observable<rpp::subjects::utils::extract_subject_type_t<Subject>>>)
+        dynamic_connectable_observable(rpp::connectable_observable<Strategy, Subject> original)
+            : base{std::move(original.get_observable()), original.get_subject()}
+        {
+        }

By accepting original by value and moving it when necessary, you handle both lvalue and rvalue cases in a single constructor, reducing code duplication.

Committable suggestion skipped: line range outside the PR's diff.

} // namespace rpp
6 changes: 1 addition & 5 deletions src/rpp/rpp/observables/dynamic_observable.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,7 @@ namespace rpp::details::observables
template<rpp::constraint::observable Observable>
static const vtable* create() noexcept
{
static vtable s_res{
.subscribe = forwarding_subscribe<Type, Observable>};
static vtable s_res{.subscribe = forwarding_subscribe<Type, Observable>};
return &s_res;
}
};
Expand Down Expand Up @@ -103,7 +102,4 @@ namespace rpp
{
}
};

template<typename Subject>
using dynamic_connectable_observable = connectable_observable<rpp::dynamic_observable<rpp::subjects::utils::extract_subject_type_t<Subject>>, Subject>;
} // namespace rpp
3 changes: 3 additions & 0 deletions src/rpp/rpp/observables/fwd.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,9 @@ namespace rpp
template<constraint::decayed_type Type>
class dynamic_observable;

template<typename Subject>
class dynamic_connectable_observable;

template<constraint::decayed_type Type, constraint::observable_strategy<Type> Strategy>
class blocking_observable;

Expand Down
3 changes: 3 additions & 0 deletions src/tests/rpp/test_connectable_observable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <doctest/doctest.h>

#include <rpp/observables/connectable_observable.hpp>
#include <rpp/observables/dynamic_connectable_observable.hpp>
#include <rpp/observables/dynamic_observable.hpp>
#include <rpp/observers/mock_observer.hpp>
#include <rpp/operators/map.hpp>
Expand Down Expand Up @@ -82,6 +83,8 @@ TEST_CASE("connectable observable")
test(rpp::connectable_observable{source, rpp::subjects::publish_subject<int>{}});
SUBCASE("dynamic_connectable created manually")
test(rpp::dynamic_connectable_observable<rpp::subjects::publish_subject<int>>{source, rpp::subjects::publish_subject<int>{}});
SUBCASE("dynamic_connectable coneverted")
test(rpp::dynamic_connectable_observable<rpp::subjects::publish_subject<int>>{rpp::connectable_observable{source, rpp::subjects::publish_subject<int>{}}});
SUBCASE("connectable created via multicast")
test(source | rpp::ops::multicast(rpp::subjects::publish_subject<int>{}));
SUBCASE("connectable created via templated multicast")
Expand Down
10 changes: 10 additions & 0 deletions src/tests/rpp/test_observables.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,20 @@ TEST_CASE("create observable works properly as observable")
test(observable.as_dynamic());
}

SUBCASE("dynamic observable cast")
{
test(rpp::dynamic_observable<int>{observable});
}

SUBCASE("dynamic observable via move")
{
test(std::move(observable).as_dynamic()); // NOLINT
}

SUBCASE("dynamic observable cast via move")
{
test(rpp::dynamic_observable<int>{std::move(observable)}); // NOLINT
}
}

TEST_CASE("blocking_observable blocks subscribe call")
Expand Down
3 changes: 3 additions & 0 deletions src/tests/rpp/test_observers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,9 @@

SUBCASE("dynamic observer")
test_observer(std::move(original_observer).as_dynamic());

SUBCASE("dynamic observer via cast")
test_observer(rpp::dynamic_observer<int>{std::move(original_observer)});

Check failure on line 193 in src/tests/rpp/test_observers.cpp

View workflow job for this annotation

GitHub Actions / tests ci-ubuntu-clang Debug

'original_observer' used after it was moved [bugprone-use-after-move,-warnings-as-errors]
}

TEST_CASE("set_upstream can be called multiple times")
Expand Down
Loading