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

Fixes cursor iteration for set_union #87

Merged
merged 2 commits into from
Jul 12, 2023

Conversation

jwest591
Copy link
Contributor

@jwest591 jwest591 commented Jul 8, 2023

last() must return a cursor with cursor_type::second otherwise comparing against a cursor obtained via successive inc() calls fails.

This change also allows flux::to work with set_union.

last() must return a cursor with cursor_type::second otherwise comparing
against a cursor obtain via successive inc() calls fails.

This change also allows flux::to work with set_union.
@codecov
Copy link

codecov bot commented Jul 9, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.19 ⚠️

Comparison is base (404b732) 97.85% compared to head (08f98af) 97.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #87      +/-   ##
==========================================
- Coverage   97.85%   97.66%   -0.19%     
==========================================
  Files          65       66       +1     
  Lines        2191     2227      +36     
==========================================
+ Hits         2144     2175      +31     
- Misses         47       52       +5     
Impacted Files Coverage Δ
include/flux/op/set_adaptors.hpp 86.11% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tcbrindle tcbrindle self-requested a review July 10, 2023 14:37
@tcbrindle
Copy link
Owner

Hi @jwest591, thanks for the PR, and in particular for all the test additions, it looks great.

@jnytra, set_union was yours originally, are you happy to merge?

@tcbrindle
Copy link
Owner

@jwest591 Please could you pull the latest upstream Flux change so we can re-test the MacOS CI? (Github Actions made some changes but things should be working again now with the latest fix)

Don't worry about the CodeCov "failure", it's a strange fact of life that with a template library and mostly constexpr tests, sometimes adding a new runtime test case can cause reported coverage to go down because more code becomes "live" in the final binary...

@tcbrindle
Copy link
Owner

@jnytra I'm going to merge this as I think the fix is good, let me know if you see any problems

@tcbrindle tcbrindle merged commit f72ceab into tcbrindle:main Jul 12, 2023
@tcbrindle
Copy link
Owner

Merged, thanks very much @jwest591

@jwest591
Copy link
Contributor Author

You're welcome! Glad it was useful.

@jnytra
Copy link
Contributor

jnytra commented Jul 13, 2023

Thanks for the fix @jwest591, it also looks good for me @tcbrindle. :-)

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.

3 participants