[MISC] various fixes related to alphabet CPOs #1225

h-2 · 2019-08-22T16:40:12Z

Major change:

We don't have namespace seqan3::custom anymore, because using that only worked if additions to it were included before the definition of the CPO.
We have template <typename t> struct seqan3::custom instead. It is specialised over the type that you are creating customisations for. Customisations are added as static member functions instead of free functions (the struct acts like "namespace template").
I have adapted all existing code to follow this and added an extra test. Including alphabet/concept doesn't automatically include the adaptation alphabets anymore, because it's not necessary.

Other changes:

Added poison pill overloads to prevent non-ADL forms of unqualified lookup.
Changed order of CPO checks. custom is now checked first, because it is the most explicit form of customisation, it should be able to overwrite the other ones.
various minor fixes and cleanup

Open design question:
We now have one type that can be specialised for all customisation points. This means if someone wants T to model alphabet, and they also want T to model ArgParserValuable (or however the thing is going to be called) then they need to provide the customisations for both concepts in the same specialisation "instance" of seqan3::custom.

We could fix this by having seqan3::custom::alphabet<T> for the alphabet customisation points and then other sub-types in seqan3::custom:: for other customisation points, but I think this is a little overkill. 90% of our users will never write their own alphabets, 90% of the remaining users will program their types with members or ADL. 1% might want to adapt a third party type, but targetting a third party type that is already overloaded by a fourth-party for a different customisation point seems very unlikely. Even then they could do it with tricks like requires true.

BUT I wanted to say it upfront.

codecov · 2019-08-22T17:44:37Z

Codecov Report

Merging #1225 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #1225   +/-   ##
=======================================
  Coverage   96.83%   96.83%           
=======================================
  Files         218      218           
  Lines        8748     8748           
=======================================
  Hits         8471     8471           
  Misses        277      277

Impacted Files	Coverage Δ
include/seqan3/core/detail/debug_stream_range.hpp	`100% <ø> (ø)`	⬆️
include/seqan3/io/structure_file/format_vienna.hpp	`97.01% <ø> (ø)`	⬆️
include/seqan3/alphabet/cigar/cigar.hpp	`100% <ø> (ø)`	⬆️
include/seqan3/io/sequence_file/format_fasta.hpp	`97.26% <ø> (ø)`	⬆️
include/seqan3/io/sequence_file/format_fastq.hpp	`98.38% <ø> (ø)`	⬆️
include/seqan3/alphabet/adaptation/uint.hpp	`100% <100%> (ø)`	⬆️
include/seqan3/alphabet/nucleotide/concept.hpp	`100% <100%> (ø)`	⬆️
include/seqan3/alphabet/adaptation/char.hpp	`100% <100%> (ø)`	⬆️
include/seqan3/alphabet/concept.hpp	`100% <100%> (ø)`	⬆️
include/seqan3/alphabet/structure/concept.hpp	`100% <100%> (ø)`	⬆️
... and 6 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 96e1db6...57af72b. Read the comment docs.

doc/about/customisation/index.md

doc/howto/write_an_alphabet/index.md

include/seqan3/alphabet/adaptation/char.hpp

include/seqan3/core/detail/customisation_point.hpp

test/unit/alphabet/custom_alphabet3_test.cpp

h-2 · 2019-08-23T10:19:22Z

Thanks for the review, what's you opinion on the design question?

smehringer · 2019-08-23T11:45:41Z

I actually like the new design better than the namespace solution. I did not see this way of doing it on the other blog posts so far, and from the ones I saw I prefer yours. But it feels like this is an area that will change over the next few years when more people start adopting this design so I wonder if it is the best way to adapt it later. This will probably be something we will consider in seqan4 (let's hope something similar to the proposal for using virtual and override on functions makes it into c++23).

doc/howto/write_an_alphabet/index.md

include/seqan3/alphabet/adaptation/char.hpp

include/seqan3/alphabet/concept.hpp

rrahn

mostly minor stuff.

rrahn · 2019-09-02T11:44:49Z

doc/howto/write_an_alphabet/index.md

+In that case, you need to specialise the seqan3::custom::alphabet class template and provide the required functionality
+as static members.
+
+\snippet test/unit/alphabet/custom_alphabet3_test.cpp third_party_type


I thought we don't use snippets anymore?

rrahn · 2019-09-02T11:48:54Z

include/seqan3/alphabet/adaptation/uint.hpp

-    return intgr = chr;
-}
+    /*!\brief Converting uint to rank is a no-op (it will just return the value you pass in).
+     * \param intgr The alphabet letter that you wish to convert to rank.


Suggested change

* \param intgr The alphabet letter that you wish to convert to rank.

* \param[in] intgr The alphabet letter that you wish to convert to rank.

and it looks like all other places

rrahn · 2019-09-02T11:52:07Z

include/seqan3/alphabet/concept.hpp

+ * \details
+ *
+ * For examples of when and how you can make use of this type, please see \link about_customisation the page on
+ * customisation \endlink and the \link howto_write_an_alphabet_custom  section on third party types \endlink in


Suggested change

* customisation \endlink and the \link howto_write_an_alphabet_custom section on third party types \endlink in

* customisation \endlink and the \link howto_write_an_alphabet_custom section on third party types \endlink in

not resolved

rrahn · 2019-09-02T11:53:56Z

include/seqan3/alphabet/concept.hpp

-    SEQAN3_CPO_IMPL(2, to_rank(v)                       )    // ADL
-    SEQAN3_CPO_IMPL(1, seqan3::custom::to_rank(v)       )    // customisation namespace
-    SEQAN3_CPO_IMPL(0, v.to_rank()                      )    // member
+    SEQAN3_CPO_IMPL(2, seqan3::custom::alphabet<decltype(v)>::to_rank(v)    )    // explicit customisation


Suggested change

SEQAN3_CPO_IMPL(2, seqan3::custom::alphabet<decltype(v)>::to_rank(v) ) // explicit customisation

SEQAN3_CPO_IMPL(2, seqan3::custom::alphabet<decltype(v)>::to_rank(v)) // explicit customisation

rrahn · 2019-09-02T11:55:21Z

include/seqan3/alphabet/concept.hpp

-    SEQAN3_CPO_IMPL(2, (assign_rank_to(args..., v)                           ))    // ADL
-    SEQAN3_CPO_IMPL(1, (seqan3::custom::assign_rank_to(args..., v)           ))    // customisation namespace
-    SEQAN3_CPO_IMPL(0, (v.assign_rank(args...)                               ))    // member
+    SEQAN3_CPO_IMPL(2, (seqan3::custom::alphabet<decltype(v)>::assign_rank_to(args..., v)   )) // explicit customisation


please remove the spaces in the end.

rrahn · 2019-09-02T13:05:10Z

include/seqan3/alphabet/structure/concept.hpp

 {

+//!\brief Poison-pill overload to prevent non-ADL forms of unqualified lookup.
+template <typename ...args_t>
+void is_pair_close(args_t ...) = delete;


ellipse version

rrahn · 2019-09-02T13:05:17Z

include/seqan3/alphabet/structure/concept.hpp

 {

+//!\brief Poison-pill overload to prevent non-ADL forms of unqualified lookup.
+template <typename ...args_t>
+void is_unpaired(args_t ...) = delete;


rrahn · 2019-09-02T13:05:31Z

include/seqan3/alphabet/structure/concept.hpp

 {

+//!\brief Poison-pill overload to prevent non-ADL forms of unqualified lookup.
+template <typename ...args_t>
+void max_pseudoknot_depth(args_t ...) = delete;


rrahn · 2019-09-02T13:05:50Z

include/seqan3/alphabet/structure/concept.hpp

 {

+//!\brief Poison-pill overload to prevent non-ADL forms of unqualified lookup.
+template <typename ...args_t>
+void pseudoknot_id(args_t ...) = delete;


rrahn · 2019-09-02T13:05:54Z

include/seqan3/alphabet/structure/concept.hpp

-    SEQAN3_CPO_IMPL(2, pseudoknot_id(v)                     ) // ADL
-    SEQAN3_CPO_IMPL(1, seqan3::custom::pseudoknot_id(v)     ) // customisation namespace
-    SEQAN3_CPO_IMPL(0, v.pseudoknot_id()                    ) // member
+    SEQAN3_CPO_IMPL(2, seqan3::custom::alphabet<decltype(v)>::pseudoknot_id(v)  ) // explicit customisation


h-2 · 2019-09-03T09:57:59Z

polite review ping

rrahn

💅 only

rrahn · 2019-09-03T11:51:39Z

include/seqan3/alphabet/concept.hpp

+ * \details
+ *
+ * For examples of when and how you can make use of this type, please see \link about_customisation the page on
+ * customisation \endlink and the \link howto_write_an_alphabet_custom  section on third party types \endlink in


not resolved

rrahn · 2019-09-03T22:15:31Z

ci is broken

rrahn

one open remark

rrahn · 2019-09-04T11:28:52Z

include/seqan3/alphabet/concept.hpp

-    SEQAN3_CPO_IMPL(2, to_rank(v)                       )    // ADL
-    SEQAN3_CPO_IMPL(1, seqan3::custom::to_rank(v)       )    // customisation namespace
-    SEQAN3_CPO_IMPL(0, v.to_rank()                      )    // member
+    SEQAN3_CPO_IMPL(2, seqan3::custom::alphabet<decltype(v)>::to_rank(v))    // explicit customisation


Just a remark. We want the user to prefer overload in the respective objects namespace if possible. But why do we ask now in this order? Would the compiler not stop when it knows it found a valid overload and now it always has to begin with the least likely scenario and therefor affect the compile time? Just a thought.

I am pretty sure the compiler needs to instantiate all IMPLs to find out what the valid overload-set is so it doesn't make a difference for compile-time.

However, it might make sense to document more clearly that we want people to provide implementations in the reverse order if possible.

h-2 requested a review from smehringer August 22, 2019 16:40

smehringer reviewed Aug 23, 2019

View reviewed changes

marehr mentioned this pull request Aug 26, 2019

[FIX] visibility errors on gcc #1228

Closed

h-2 force-pushed the cpo_the_last_one branch from 27b73d3 to 91700f8 Compare August 29, 2019 13:54

h-2 requested a review from smehringer August 29, 2019 13:55

smehringer requested changes Aug 29, 2019

View reviewed changes

h-2 force-pushed the cpo_the_last_one branch from 91700f8 to abac33d Compare August 29, 2019 16:24

h-2 requested a review from smehringer August 29, 2019 16:27

smehringer approved these changes Aug 30, 2019

View reviewed changes

h-2 requested a review from rrahn August 30, 2019 08:54

rrahn requested changes Sep 2, 2019

View reviewed changes

h-2 force-pushed the cpo_the_last_one branch from abac33d to bc95acd Compare September 2, 2019 14:15

h-2 requested a review from rrahn September 2, 2019 14:15

rrahn requested changes Sep 3, 2019

View reviewed changes

[MISC] various fixes related to alphabet CPOs

1abf2c5

h-2 force-pushed the cpo_the_last_one branch from bc95acd to 1abf2c5 Compare September 3, 2019 12:11

h-2 requested a review from rrahn September 3, 2019 12:11

Merge branch 'master' into cpo_the_last_one

dcfb7fa

rrahn approved these changes Sep 4, 2019

View reviewed changes

fix stuff

57af72b

h-2 merged commit db42822 into seqan:master Sep 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MISC] various fixes related to alphabet CPOs #1225

[MISC] various fixes related to alphabet CPOs #1225

h-2 commented Aug 22, 2019

codecov bot commented Aug 22, 2019 •

edited

h-2 commented Aug 23, 2019

smehringer commented Aug 23, 2019 •

edited

rrahn left a comment

rrahn Sep 2, 2019

rrahn Sep 2, 2019

rrahn Sep 2, 2019

rrahn Sep 3, 2019

rrahn Sep 2, 2019

rrahn Sep 2, 2019

rrahn Sep 2, 2019

rrahn Sep 2, 2019

rrahn Sep 2, 2019

rrahn Sep 2, 2019

rrahn Sep 2, 2019

h-2 commented Sep 3, 2019

rrahn left a comment

rrahn Sep 3, 2019

rrahn commented Sep 3, 2019

rrahn left a comment

rrahn Sep 4, 2019

h-2 Sep 4, 2019

	* \param intgr The alphabet letter that you wish to convert to rank.
	* \param[in] intgr The alphabet letter that you wish to convert to rank.

	* customisation \endlink and the \link howto_write_an_alphabet_custom section on third party types \endlink in
	* customisation \endlink and the \link howto_write_an_alphabet_custom section on third party types \endlink in

	SEQAN3_CPO_IMPL(2, seqan3::custom::alphabet<decltype(v)>::to_rank(v) ) // explicit customisation
	SEQAN3_CPO_IMPL(2, seqan3::custom::alphabet<decltype(v)>::to_rank(v)) // explicit customisation

[MISC] various fixes related to alphabet CPOs #1225

[MISC] various fixes related to alphabet CPOs #1225

Conversation

h-2 commented Aug 22, 2019

codecov bot commented Aug 22, 2019 • edited

Codecov Report

h-2 commented Aug 23, 2019

smehringer commented Aug 23, 2019 • edited

rrahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-2 commented Sep 3, 2019

rrahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrahn commented Sep 3, 2019

rrahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 22, 2019 •

edited

smehringer commented Aug 23, 2019 •

edited