Skip to content

[FIX] Restrict seqan3::alphabet_variant. - #2868

Merged
eseiler merged 2 commits into
seqan:release-3.1.0from
smehringer:alphabet_fix
Nov 4, 2021
Merged

[FIX] Restrict seqan3::alphabet_variant.#2868
eseiler merged 2 commits into
seqan:release-3.1.0from
smehringer:alphabet_fix

Conversation

@smehringer

Copy link
Copy Markdown
Member

to only accept alphabets with the same underlying char type.

This cannot be tested, since incorrect code would not compile.

since all our alphabets have char as underlying type, this did not break anything.

Fixes #2867

@vercel

vercel Bot commented Oct 27, 2021

Copy link
Copy Markdown

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/88MY8wv4L92v2DecrzYsw4WcJM7o
✅ Preview: https://seqan3-git-fork-smehringer-alphabetfix-seqan.vercel.app

@codecov

codecov Bot commented Oct 27, 2021

Copy link
Copy Markdown

Codecov Report

Merging #2868 (84a8da4) into release-3.1.0 (3a9932f) will increase coverage by 98.21%.
The diff coverage is n/a.

Impacted file tree graph

@@                Coverage Diff                 @@
##           release-3.1.0    #2868       +/-   ##
==================================================
+ Coverage               0   98.21%   +98.21%     
==================================================
  Files                  0      272      +272     
  Lines                  0    10827    +10827     
==================================================
+ Hits                   0    10634    +10634     
- Misses                 0      193      +193     
Impacted Files Coverage Δ
...ude/seqan3/alphabet/composite/alphabet_variant.hpp 100.00% <ø> (ø)
...e/seqan3/alignment/scoring/scoring_scheme_base.hpp 100.00% <0.00%> (ø)
...qan3/alignment/pairwise/alignment_configurator.hpp 97.67% <0.00%> (ø)
include/seqan3/core/debug_stream/optional.hpp 100.00% <0.00%> (ø)
include/seqan3/utility/tuple/split.hpp 100.00% <0.00%> (ø)
...seqan3/io/structure_file/output_format_concept.hpp 100.00% <0.00%> (ø)
include/seqan3/io/sequence_file/output.hpp 100.00% <0.00%> (ø)
...n3/alignment/scoring/nucleotide_scoring_scheme.hpp 100.00% <0.00%> (ø)
include/seqan3/io/record.hpp 100.00% <0.00%> (ø)
include/seqan3/search/search_result.hpp 100.00% <0.00%> (ø)
... and 263 more

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 3a9932f...84a8da4. Read the comment docs.

@smehringer
smehringer force-pushed the alphabet_fix branch 2 times, most recently from fdea6f3 to 865a759 Compare October 27, 2021 09:21
@smehringer
smehringer requested review from a team and remyschwab and removed request for a team October 27, 2021 09:48
@marehr

marehr commented Oct 27, 2021

Copy link
Copy Markdown
Member

This cannot be tested, since incorrect code would not compile.

Fun fact you can write a concept that checks whether the type definition works.

template <typename ...alphabets_t>
concept alphabet_variant_can_be_declared = requires()
{
  typename seqan3::alphabet_variant<alphabets_t...>;
};

Proof of concept: https://godbolt.org/z/rEKcoehqd

since all our alphabets have char as underlying type, this did not break anything.

Not completely true, since std::variant<uint8_t, uint16_t> won't be allowed any more. Or something like std::variant<signed char, unsigned char> might not work any more.

The fix now shows that there are 4 possible ways to address the char situation:

  1. Always use char (simplest), (nearly) all alphabet combinations will still work, except that the representable char domain will be limited to 256 chars. Assumes that all char types are convertible to char. (behaviour before)
  2. assume that all alphabets have the same char domain, most alphabet combinations will work, guarantees the same char domain as the alphabets (behaviour of this PR)
  3. always use the char representation of the first alphabet, (nearly) all alphabet combinations will still work, except that the representable char domain will be limited to that of the first alphabet. Assumes that all char types are convertible to the first one. (I think I implemented this behaviour once, but it was changed by Hannes to 1. to simplify implementation)
  4. use the common char representation of all alphabets (see std::common_type), (nearly) all alphabet combinations will still work. It assumes that all alphabets have a compatible char domain and selects the biggest of them, for example <char, char16_t, char, char16_t> should select char16_t as common type. (No one tried yet, but I would be in favour of this one if it works)

@smehringer

Copy link
Copy Markdown
Member Author

Core Meeting 01.11.2021:

We will restrict the seqan3::alphabet_variant to only work on chars. If we support wide chars can be tested once the use case arrives.

@smehringer
smehringer force-pushed the alphabet_fix branch 2 times, most recently from aabf377 to a0786f1 Compare November 1, 2021 11:10
@smehringer
smehringer requested review from a team and Irallia and removed request for a team November 2, 2021 06:56

@Irallia Irallia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGFM, just a spelling mistake.

Comment thread include/seqan3/alphabet/composite/alphabet_variant.hpp Outdated
@smehringer
smehringer requested a review from Irallia November 2, 2021 10:49
@Irallia
Irallia requested review from a team and eseiler and removed request for a team November 2, 2021 14:58
@SGSSGene SGSSGene added this to the SeqAn 3.1.0 milestone Nov 4, 2021
@smehringer
smehringer changed the base branch from master to release-3.1.0 November 4, 2021 10:22
Comment thread include/seqan3/alphabet/composite/alphabet_variant.hpp Outdated
Comment thread include/seqan3/alphabet/composite/alphabet_variant.hpp Outdated
@eseiler
eseiler enabled auto-merge (squash) November 4, 2021 10:55
@eseiler
eseiler merged commit cd692e3 into seqan:release-3.1.0 Nov 4, 2021
eaasna pushed a commit to eaasna/seqan3 that referenced this pull request Feb 21, 2022
* [FIX] Restrict seqan3::alphabet_variant to only accept alphabets with the same underlying char type.

* Apply suggestions from code review

Co-authored-by: Enrico Seiler <eseiler@users.noreply.github.com>
@smehringer
smehringer deleted the alphabet_fix branch November 27, 2023 07:01
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.

[Alphabet] Open Todo: Contraing alphabet_variant to only allow types with the same underlying char type.

5 participants