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

Final refactoring of alphabets #869

Closed
8 tasks done
h-2 opened this issue Apr 2, 2019 · 9 comments
Closed
8 tasks done

Final refactoring of alphabets #869

h-2 opened this issue Apr 2, 2019 · 9 comments

Comments

@h-2
Copy link
Member

h-2 commented Apr 2, 2019

These are the discussed changes and some smaller additions by me not yet discussed.
@seqan/core please have a look and give me objections ASAP since I want to start working on this very soon.

seqan3::SemialphabetConcept:

  • refines std::StrictTotallyOrdered (order required to respond to ranks)
  • refines std::Copyable and requires std::is_trivially_copyable
  • requirement for std::Regular is dropped, but recommended for all type definitions
  • to_rank customisation point object
    • looks for member function
    • looks for free function via ADL
    • not picked up via ADL itself
  • alphabet_size_v
    • looks for static member variable ::alphabet_size (now it's ::value_size – a weird name leftover from SeqAn2)
    • looks for constexpr free function via ADL alphabet_size() which makes it possible to define your own alphabet without overriding the metafunction in SeqAn

seqan3::WritableSemialphabetConcept:

  • refines seqan3::SemialphabetConcept
  • assign_rank_to(rank, alph) customisation point object
    • was assign_rank(alph, rank) in seqan2-(target, source)-style; new style is more intuitive and allows for assign_rank_to<dna5>(rank) instead of explicit construction assign_rank(dna5{}, rank) [also necessary because we don't require regularity anymore]
    • looks for assign_rank(rank) member function
    • looks for assign_rank_to(rank, alph) free function via ADL
    • not picked up via ADL itself
    • the CPO defines a generic overload for assign_rank_to(rank, alph &&) so this does not have to be customised (only the overload for lvalue-references has to be customised).

seqan3::AlphabetConcept

  • refines seqan3::SemialphabetConcept
  • to_char customisation point analogous to above

seqan3::WritableAlphabetConcept

  • refines seqan3::AlphabetConcept and seqan3::WritableSemialphabetConcept
  • assign_char_to(char, alph) customisation point object analogous to assign_rank_to(rank, alph)

PLUS Constexpr versions of all of the above in seqan3::detail::.

  • Note that underlying_rank_t and underlying_char_t will be renamed to alphabet_rank_t and alphabet_char_t and will not be customisation points, they always resolve to the return type of to_char() and `to_rank().

  • rename alphabet_size_v -> alphabet_size

  • rename ::value_size -> alphabet_size

@eseiler
Copy link
Member

eseiler commented Apr 2, 2019

In general I agree. I'm just wondering why we are dropping the std::Regular requirement. How will we adjust all the, e.g., unit tests where we require to default construct a alphabet?

@h-2
Copy link
Member Author

h-2 commented Apr 2, 2019

We don't have to adapt our tests because our alphabets will still be Regular. We just don't require it.

The reason was, as discussed in escala, that default constructability is not required by interfaces that only consume alphabets and that it prevents references from modelling the concepts.

@smehringer
Copy link
Member

Hm... didn't we have the function style guide for parameters (out, in/out, in)? the proposed changes to the free functions would be inconsistent with this..

@h-2
Copy link
Member Author

h-2 commented Apr 3, 2019

Hm... didn't we have the function style guide for parameters (out, in/out, in)? the proposed changes to the free functions would be inconsistent with this..

No, that was SeqAn2-style. In SeqAn3 it's "natural order" and has been since day 1:
https://github.com/seqan/seqan3/wiki/Functions#function-argument-number-and-order

@smehringer
Copy link
Member

Then I'm fine with the changes

@eseiler
Copy link
Member

eseiler commented Apr 4, 2019

We don't have to adapt our tests because our alphabets will still be Regular. We just don't require it.

So we implicitly require it for our alphabets then.

The reason was, as discussed in escala, that default constructability is not required by interfaces that only consume alphabets and that it prevents references from modelling the concepts.

Ok.

I agree with the changes.

@rrahn
Copy link
Contributor

rrahn commented Apr 9, 2019

looks good to me.

@rrahn
Copy link
Contributor

rrahn commented Jun 5, 2019

is this done?

@h-2
Copy link
Member Author

h-2 commented Jun 5, 2019

Yep

@h-2 h-2 closed this as completed Jun 5, 2019
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

No branches or pull requests

4 participants