implement filter_map

[Guekka]

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #192   +/-   ##
=======================================
  Coverage   98.22%   98.22%           
=======================================
  Files          69       69           
  Lines        2360     2360           
=======================================
  Hits         2318     2318           
  Misses         42       42           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tcbrindle]

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]

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

[Guekka]

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

[tcbrindle]

This is looking good. I've suggested a few changes but they're pretty minor.

I the long term, I'd prefer filter_map to have its own dedicated adaptor class rather than using a chain of three existing adaptors, but doing it this way is fine for now.

We also need some documentation (or at least placeholders) for filter_map and filter_deref in docs/reference/adaptors.rst to let people know that they exist. If this is tricky then let me know and I can add it to the PR before merging.

[tcbrindle]

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

[Guekka]

Resolved reviews. Still have to write doc

[Guekka]

Please review thoroughly the docs @tcbrindle, I'm not entirely sure of what I wrote
Also, thank you for sticking with me, it would probably have been faster for you to write it all

I will rebase the PR once you approve it

[tcbrindle]

Good job on the documentation, I've made some suggestions to add a bit more description but otherwise it's good. I like making the doc target watch all the .rst files 👍

There's one change I don't understand with FLUX_NO_UNIQUE_ADDRESS, if this wasn't intentional then please revert it (or else let me know why it's needed)

Other than that it's just some minor formatting things

[Guekka]

Fixed. A clang format file would really be helpful, even if it means losing some custom formatting, I think it would be worth it for contributors

[tcbrindle]

Just one tiny change to silence a Sphinx warning and then we're good to go 👍

[tcbrindle]

Thanks for your work on this @Guekka!