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

[MISC] relax kmer_hash_view::iterator difference requirement #2931

Merged
merged 1 commit into from
Jan 25, 2022

Conversation

marehr
Copy link
Member

@marehr marehr commented Jan 24, 2022

The difference operator on iterators only needs the requirement std::sized_sentinel_for<it_t, it_t>, i.e. the underlying iterator has a difference operator, instead of the full std::random_access_iterator.

The random_access_iterator requires that as a subclause

    std::totally_ordered<I> &&
    std::sized_sentinel_for<I, I> && // here
    requires(I i, const I j, const std::iter_difference_t<I> n) {

An example from the standard would be here: https://eel.is/c++draft/range.adaptors#range.transform.iterator-22 where it also only requires std::sized_sentinel_for.

@marehr marehr requested review from a team and eaasna and removed request for a team January 24, 2022 00:45
@vercel
Copy link

vercel bot commented Jan 24, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/seqan/seqan3/A5oCgsMrXRvNNQGfCBhuszbcbqRz
✅ Preview: https://seqan3-git-fork-marehr-kmerhash-seqan.vercel.app

@codecov
Copy link

codecov bot commented Jan 24, 2022

Codecov Report

Merging #2931 (137891f) into master (f668060) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2931   +/-   ##
=======================================
  Coverage   98.28%   98.28%           
=======================================
  Files         267      267           
  Lines       11462    11462           
=======================================
  Hits        11265    11265           
  Misses        197      197           
Impacted Files Coverage Δ
include/seqan3/search/views/kmer_hash.hpp 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f668060...137891f. Read the comment docs.

@marehr marehr requested review from a team and smehringer and removed request for a team January 24, 2022 13:18
Copy link
Member

@smehringer smehringer left a comment

Choose a reason for hiding this comment

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

Makes sense since we only compute the difference and this concept checks "whether you can compute the distance".

I guess this holds for more of our views but I would alter them if the use case arises.

@smehringer smehringer merged commit 58c7109 into seqan:master Jan 25, 2022
@marehr
Copy link
Member Author

marehr commented Jan 25, 2022

I guess this holds for more of our views but I would alter them if the use case arises.

We could still make this a follow-up easy to-do issue. Searching for operator- and checking whether the iterator difference operator has the right restriction, shouldn't be too hard.

@marehr marehr deleted the kmer_hash branch January 25, 2022 10:02
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