Implement flat_map operator #374

CorentinBT · 2023-06-10T10:31:28Z

Should I also implement a variant taking a ResultSelector like in rxcpp?

CorentinBT · 2023-06-10T10:32:46Z

src/tests/rpp/test_flat_map.cpp

+    {
+        copy_count_tracker verifier{};
+        auto obs = rpp::source::just<TestType>(std::move(verifier))
+                 | rpp::ops::flat_map([](copy_count_tracker&& verifier) { return rpp::source::just<TestType>(std::move(verifier)); });


@victimsnino do you know why sources don't move objects through operators? Preventing me from taking verifier as an rvalue ref here

Because it would be moved IF possible, but no guarantees. If it would be only pure rvalue, then it means second repeated subscribe would see nothing :)

I see, I added some cases for copies then

if you would use create with std::move inside, then it would be moved inside lambda

victimsnino · 2023-06-10T15:02:49Z

src/benchmarks/benchmarks.cpp

+            TEST_RPP([&]()
+            {
+                rpp::source::create<int>([](const auto& obs){ obs.on_next(1); })
+                    | rpp::operators::flat_map([](int v) { return rpp::source::just(v * 2); })


Could you use “create” there instead of just to measure there only “flat_map”, not “just” ? :)

codecov · 2023-06-10T15:03:22Z

Codecov Report

Merging #374 (45e0ec6) into v2 (05a4d05) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##               v2     #374      +/-   ##
==========================================
+ Coverage   99.12%   99.13%   +0.01%     
==========================================
  Files          37       38       +1     
  Lines         802      812      +10     
==========================================
+ Hits          795      805      +10     
  Misses          7        7

Impacted Files	Coverage Δ
src/rpp/rpp/operators/flat_map.hpp	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

victimsnino · 2023-06-10T15:04:19Z

src/rpp/rpp/operators/flat_map.hpp

+    RPP_NO_UNIQUE_ADDRESS Fn m_fn;
+
+    template<rpp::constraint::observable TObservable>
+        requires std::invocable<Fn, rpp::utils::extract_observable_type_t<TObservable>>


There is also invocation result must be observable

It is already required here isn't it enough?

No, check this comment

https://github.com/victimsnino/ReactivePlusPlus/pull/374/files#r1225415597

victimsnino · 2023-06-10T15:05:10Z

src/rpp/rpp/operators/flat_map.hpp

+    auto operator()(TObservable&& observable) &&
+    {
+        return std::forward<TObservable>(observable)
+                | rpp::ops::map(std::forward<Fn>(m_fn))


Should be move

victimsnino · 2023-06-10T15:07:47Z

src/rpp/rpp/operators/fwd.hpp

@@ -33,6 +33,10 @@ template<typename Fn>
    requires (!utils::is_not_template_callable<Fn> || !std::same_as<void, std::invoke_result_t<Fn, utils::convertible_to_any>>)
 auto map(Fn&& callable);

+template<typename Fn>
+    requires rpp::constraint::observable<std::invoke_result_t<Fn, utils::convertible_to_any>>


Add check over !utils::is_not_template_callable as in map. It needed when passed function is purely template. For example [](auto v) {return rpp::source::just(v); } wouldn't satisfy your requirement

done, added a test too

victimsnino · 2023-06-10T17:59:12Z

src/rpp/rpp/operators/flat_map.hpp

@@ -22,8 +22,8 @@ struct flat_map_t
 {
    RPP_NO_UNIQUE_ADDRESS Fn m_fn;

-    template<rpp::constraint::observable TObservable>
-        requires std::invocable<Fn, rpp::utils::extract_observable_type_t<TObservable>>
+    template<rpp::constraint::observable TObservable, typename ValueType = rpp::utils::extract_observable_type_t<TObservable>>


I see your attempt with ValueType to avoid duplicates, but it would affect final binaries due to it would be compiled-in as template function with 2 params instead of one =C it is better to create special concept instead

victimsnino · 2023-06-10T18:01:22Z

src/tests/rpp/test_flat_map.cpp

@@ -105,22 +106,43 @@ TEMPLATE_TEST_CASE("flat_map", "", rpp::memory_model::use_stack, rpp::memory_mod
                CHECK(mock.get_on_error_count() == 1);
            }
        }
+        SECTION("subscribe using flat_map with templated lambda")


i think, you can just place it in a748a6f#diff-4ad61bca345fc2ef117d5617e164dca71340d801c636340f2c051a5bd4e6e9d2R37

victimsnino · 2023-06-10T18:02:42Z

src/tests/rpp/test_flat_map.cpp

+        auto obs = rpp::source::create<copy_count_tracker>([verifier = std::move(verifier)](const auto& obs) { obs.on_next(verifier); })
+                 | rpp::ops::map([](copy_count_tracker verifier) { return std::move(verifier); }) // copy from source to map


try instead

auto obs = rpp::source::create<copy_count_tracker>([verifier = std::move(verifier)](const auto& obs) { obs.on_next(std::move(verifier)); })

Though I subscribe two times here, so verifier will be in an invalid state in second subscription as it was already moved no?

I changed to capture by ref and pass by value

Though I subscribe two times here, so verifier will be in an invalid state in second subscription as it was already moved no?

No, verifier is special class that is valid after move =)

victimsnino · 2023-06-11T15:42:38Z

src/tests/rpp/test_flat_map.cpp

+                CHECK(mock.get_on_error_count() == 0);
+            }
+        }
+        SECTION("subscribe using flat_map with error in middle")


Could you also add test over never in middle? to check that it is actually map + merge, not map + concat =)

victimsnino · 2023-06-11T15:49:21Z

src/tests/rpp/test_flat_map.cpp

+    }
+}
+
+TEST_CASE("flat_map copies/moves")


I think, you can remove this test_case due to you are trying to test map + test merge at the same time, it is pretty hard. It is better to have corresponding tests to map and merge. I'm going to add them later.

yeah agree, done

victimsnino · 2023-06-11T16:51:28Z

src/rpp/rpp/operators/flat_map.hpp

+    template<rpp::constraint::observable TObservable>
+        requires (std::invocable<Fn, rpp::utils::extract_observable_type_t<TObservable>> 
+                    && rpp::constraint::observable<std::invoke_result_t<Fn, rpp::utils::extract_observable_type_t<TObservable>>>)
+    auto operator()(TObservable&& observable) const &


Can you add test to cover this overloading? (it is enough to apply operator from variable in any test case)

victimsnino · 2023-06-11T17:52:59Z

Thanks for PR =)

Implement flat_map operator

4f58a84

CorentinBT commented Jun 10, 2023

View reviewed changes

victimsnino reviewed Jun 10, 2023

View reviewed changes

Address comments

a748a6f

victimsnino reviewed Jun 10, 2023

View reviewed changes

Corentin and others added 4 commits June 11, 2023 16:21

Address comments

bd37f38

Remove whitespace

47e25dd

Added comments

9e99db8

add marble

aba97fa

victimsnino reviewed Jun 11, 2023

View reviewed changes

Address comments

627a88e

victimsnino previously approved these changes Jun 11, 2023

View reviewed changes

victimsnino reviewed Jun 11, 2023

View reviewed changes

Increase code coverage

45e0ec6

CorentinBT dismissed victimsnino’s stale review via 45e0ec6 June 11, 2023 16:55

victimsnino approved these changes Jun 11, 2023

View reviewed changes

victimsnino merged commit 2e54fc4 into victimsnino:v2 Jun 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement flat_map operator #374

Implement flat_map operator #374

CorentinBT commented Jun 10, 2023 •

edited

Loading

CorentinBT Jun 10, 2023

victimsnino Jun 10, 2023

CorentinBT Jun 10, 2023

victimsnino Jun 10, 2023 •

edited

Loading

victimsnino Jun 10, 2023 •

edited

Loading

CorentinBT Jun 10, 2023

codecov bot commented Jun 10, 2023 •

edited

Loading

victimsnino Jun 10, 2023

CorentinBT Jun 10, 2023

victimsnino Jun 10, 2023

CorentinBT Jun 10, 2023

victimsnino Jun 10, 2023

CorentinBT Jun 10, 2023

victimsnino Jun 10, 2023

CorentinBT Jun 10, 2023

victimsnino Jun 10, 2023

CorentinBT Jun 11, 2023

victimsnino Jun 10, 2023

CorentinBT Jun 11, 2023

victimsnino Jun 10, 2023

CorentinBT Jun 11, 2023

CorentinBT Jun 11, 2023

victimsnino Jun 11, 2023

victimsnino Jun 11, 2023

CorentinBT Jun 11, 2023

victimsnino Jun 11, 2023

CorentinBT Jun 11, 2023

victimsnino Jun 11, 2023

CorentinBT Jun 11, 2023

victimsnino commented Jun 11, 2023

		auto obs = rpp::source::create<copy_count_tracker>([verifier = std::move(verifier)](const auto& obs) { obs.on_next(verifier); })
		\| rpp::ops::map([](copy_count_tracker verifier) { return std::move(verifier); }) // copy from source to map

Implement flat_map operator #374

Implement flat_map operator #374

Conversation

CorentinBT commented Jun 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victimsnino Jun 10, 2023 • edited Loading

Choose a reason for hiding this comment

victimsnino Jun 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 10, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victimsnino commented Jun 11, 2023

CorentinBT commented Jun 10, 2023 •

edited

Loading

victimsnino Jun 10, 2023 •

edited

Loading

victimsnino Jun 10, 2023 •

edited

Loading

codecov bot commented Jun 10, 2023 •

edited

Loading