tcbrindle / flux

A C++20 library for sequence-orientated programming
https://tristanbrindle.com/flux/
Boost Software License 1.0
441 stars 28 forks source link

implement filter_map #192

Open Guekka opened 1 week ago

Guekka commented 1 week ago

Adresses #191 The functionality is there, but I am still trying to understand why the static_assert fail

codecov[bot] commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.22%. Comparing base (d88fd97) to head (9a1f911).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #192 +/- ## ======================================= Coverage 98.22% 98.22% ======================================= Files 69 69 Lines 2360 2360 ======================================= Hits 2318 2318 Misses 42 42 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tcbrindle commented 1 day ago

Thanks for working on this @Guekka.

The functionality is there, but I am still trying to understand why the static_assert fail

The function is_even_opt in test_filter_map.cpp returns a prvalue std::optional<int>:

https://github.com/Guekka/flux/blob/95decaf4aeb18506804f858129b3989f32bebcab/test/test_filter_map.cpp#L15

When reading from the pipeline, the temporary optional gets dereferenced in this lambda:

https://github.com/Guekka/flux/blob/95decaf4aeb18506804f858129b3989f32bebcab/include/flux/op/filter_map.hpp#L26

This returns a reference to the temporary optional's internal int. Unfortunately, when the read_at() call returns, the temporary optional gets destroyed, and the returned reference is immediately dangling. (This took me a little while to figure out, but I'm very pleased that the constexpr testing picked it up!)

One solution is to try to detect the problem, by noticing when dereffing the optional-like would return an rvalue reference, and returning by value instead. Like so:

struct filter_map_fn {
    // If dereffing the optional would give us an rvalue reference,
    // prevent a probable dangling reference by returning by value instead
    template <typename T>
    using strip_rvalue_ref_t = std::conditional_t<
        std::is_rvalue_reference_v<T>, std::remove_reference_t<T>, T>;

   template <adaptable_sequence Seq, typename Func>
        requires (std::invocable<Func&, element_t<Seq>> &&
                  optional_like<std::remove_cvref_t<std::invoke_result_t<Func&, element_t<Seq>>>>)
    constexpr auto operator()(Seq&& seq, Func func) const
    {
        return flux::map(FLUX_FWD(seq), std::move(func))
            .filter([](auto&& opt) { return static_cast<bool>(opt); })
            .map([](auto&& opt) -> strip_rvalue_ref_t<decltype(*FLUX_FWD(opt))> {
                return *FLUX_FWD(opt);
            });
    }
};

Making this change fixes the original lifetime problem. Unfortunately this static_assert still fails because some news later in the test are never deleted, but this is pretty easy to fix :)

Hope this helps!

Guekka commented 1 day ago

Thank you @tcbrindle. Debugging why something is not constexpr is complex

Guekka commented 21 hours ago

Cannot reproduce the CI failure locally on Windows. Will try later on Linux

tcbrindle commented 20 hours ago

Cannot reproduce the CI failure locally on Windows. Will try later on Linux

Yeah, it seems like it only happens with libstdc++ -- both MSVC and libc++ pull in std::optional from somewhere else