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

implement filter_map #192

Merged
merged 2 commits into from
Jul 19, 2024
Merged

implement filter_map #192

merged 2 commits into from
Jul 19, 2024

Conversation

Guekka
Copy link
Contributor

@Guekka Guekka commented Jul 5, 2024

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

Copy link

codecov bot commented Jul 5, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

Guekka commented Jul 17, 2024

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

@Guekka Guekka marked this pull request as ready for review July 17, 2024 09:09
@Guekka Guekka force-pushed the main branch 4 times, most recently from 512a731 to 9a1f911 Compare July 17, 2024 09:13
@Guekka Guekka requested a review from tcbrindle July 17, 2024 09:14
@Guekka
Copy link
Contributor Author

Guekka commented Jul 17, 2024

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

Copy link
Owner

@tcbrindle tcbrindle left a comment

Choose a reason for hiding this comment

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

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.

test/test_filter_map.cpp Show resolved Hide resolved
include/flux/core/inline_sequence_base.hpp Outdated Show resolved Hide resolved
include/flux/core/inline_sequence_base.hpp Outdated Show resolved Hide resolved
include/flux/op/filter_map.hpp Outdated Show resolved Hide resolved
include/flux/op/filter_map.hpp Outdated Show resolved Hide resolved
test/test_filter_map.cpp Outdated Show resolved Hide resolved
test/test_filter_map.cpp Outdated Show resolved Hide resolved
test/test_filter_map.cpp Outdated Show resolved Hide resolved
@tcbrindle
Copy link
Owner

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
Copy link
Contributor Author

Guekka commented Jul 18, 2024

Resolved reviews. Still have to write doc

@Guekka
Copy link
Contributor Author

Guekka commented Jul 18, 2024

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

Copy link
Owner

@tcbrindle tcbrindle left a comment

Choose a reason for hiding this comment

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

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

include/flux/core/concepts.hpp Outdated Show resolved Hide resolved
include/flux/macros.hpp Outdated Show resolved Hide resolved
include/flux/op/filter_map.hpp Show resolved Hide resolved
docs/reference/adaptors.rst Outdated Show resolved Hide resolved
docs/reference/adaptors.rst Outdated Show resolved Hide resolved
docs/reference/adaptors.rst Outdated Show resolved Hide resolved
docs/reference/adaptors.rst Outdated Show resolved Hide resolved
@Guekka
Copy link
Contributor Author

Guekka commented Jul 18, 2024

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

@Guekka Guekka requested a review from tcbrindle July 19, 2024 07:01
Copy link
Owner

@tcbrindle tcbrindle left a comment

Choose a reason for hiding this comment

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

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

docs/reference/adaptors.rst Outdated Show resolved Hide resolved
Guekka and others added 2 commits July 19, 2024 11:53
@tcbrindle tcbrindle merged commit a6bb039 into tcbrindle:main Jul 19, 2024
37 checks passed
@tcbrindle
Copy link
Owner

Thanks for your work on this @Guekka!

@Guekka Guekka mentioned this pull request Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants