Create a version of pipeable_config_element without value #1871

joergi-w · 2020-06-03T16:48:53Z

In this way we can introduce alternative config variables and simply remove the true template argument when it's done.

Discussed in the meeting on 2nd June.

eseiler

Thank you!

I would suggest to invert the default, so you don't need to adjust all existing configurations.

CHANGELOG.md

include/seqan3/core/algorithm/concept.hpp

include/seqan3/core/algorithm/configuration.hpp

marehr · 2020-06-04T10:30:12Z

I think this must go back to the drawing board, because it will not work as intended:

https://godbolt.org/z/YjFyZc

The main problem is that we all assumed that an empty base class must not be explicitly brace initialised. The main problem is that the memory order is always {{base_class1}, {base_class2}, ..., {base_classn}, member1, member2, ...} and in brace initialisation you can only fill from left to right.

The exception to the rule would be designated intialisers {.member1{}}

We can't keep this an aggregate type if we want to keep the base class approach.

We could make them aggregate, but we would need remove the base class and thus change the piping syntax (over concepts).
We could keep the base class, making them non-aggregate, but we would need to add constructors. (note: the different issue with the piping; the base class shouldn't be allowed to be constructed)

marehr · 2020-06-07T18:20:48Z

@rrahn used in #1876 the constructor variant, see

8cec061#diff-f9b7ceceaf010f2731caabb17553d00aR72-R73

joergi-w · 2020-06-08T10:41:17Z

Are you discussing this today in the core meeting? Please let me know if there is a decision and then I can implement it accordingly.

marehr · 2020-06-08T11:07:35Z

Core-Meeting 2020-06-08:

Configs will add constructors for their members

marehr

These were the changes that we wanted and were decided in the meeting. Thank you so much!

include/seqan3/core/algorithm/pipeable_config_element.hpp

joergi-w · 2020-06-09T14:08:12Z

Rebased on master.

eseiler

Somewhere the docs reference the now missing value; thats why documentation is still failing

marehr · 2020-06-09T20:47:35Z

Travis says:

seqan3/include/seqan3/core/type_list/traits.hpp:40:

/*!\brief Implementation for seqan3::pack_traits::find_if.
 * \tparam pred_t   The predicate that is being evaluated.
 * \tparam pack_t   The type pack.
 * \returns The position of the first type `t` in `pack_t` for whom `pred_t<t>::value` is true.
 * \ingroup type_list
 */

It seems that pred_t<t>::value is associated with configuration::value xD

Let's look in the docs: https://docs.seqan.de/seqan/3-master-dev/group__type__list.html#ga5246fc497be7a3b38e42b804f35bcd7e

Indeed 😆

Not sure how to escape this class member name with doxygen.

Just wanted to show waht to change

eseiler · 2020-06-09T21:26:48Z

It seems that pred_t<t>::value is associated with configuration::value xD

Let's look in the docs: https://docs.seqan.de/seqan/3-master-dev/group__type__list.html#ga5246fc497be7a3b38e42b804f35bcd7e

Indeed 😆

Not sure how to escape this class member name with doxygen.

%pred_t<t>::value should work. % disables link generation.
But you should look at the generated docs to make sure it's correct

joergi-w · 2020-06-09T22:05:19Z

%pred_t<t>::value should work. % disables link generation.
But you should look at the generated docs to make sure it's correct

Thank you pointing this out! I'm going to fix it.

joergi-w · 2020-06-10T10:01:37Z

I could fix the documentation with the % sign and double backticks. The output website looks good. Now the Travis CI is green, but Jenkins reports red, although the tests were successful.

eseiler · 2020-06-10T10:03:44Z

Thanks for checking the docs 👍

Jenkins fail is unrelated.

rrahn

Thanks for working on this. Looks good. Some suggestion and one addition I would like to have.

rrahn · 2020-06-10T15:25:34Z

include/seqan3/core/type_list/traits.hpp

@@ -38,7 +38,7 @@ constexpr ptrdiff_t find()
 /*!\brief Implementation for seqan3::pack_traits::find_if.
 * \tparam pred_t   The predicate that is being evaluated.
 * \tparam pack_t   The type pack.
- * \returns The position of the first type `t` in `pack_t` for whom `pred_t<t>::value` is true.
+ * \returns The position of the first type `t` in `pack_t` for whom ``pred_t<t>::%value`` is true.


Suggested change

* \returns The position of the first type `t` in `pack_t` for whom ``pred_t<t>::%value`` is true.

* \returns The position of the first type `t` in `pack_t` for whom `pred_t<t>\::value` is true.

?

No sorry, this gives a warning:

seqan3/include/seqan3/core/type_list/traits.hpp:40: warning: explicit link request to 'value' could not be resolved

I keep the double backticks and percent sign, because the output is good and there are no warnings.

rrahn · 2020-06-10T15:33:59Z

include/seqan3/core/algorithm/concept.hpp

@@ -57,19 +57,14 @@ class config_element_base;
 /*!\var id
 * \brief Algorithm specific static id used for internal validation checks.
 */
-/*!\var value
- * \brief Member storing the configuration value.
- */
 //!\}
 //!\cond
 template <typename config_t>
 SEQAN3_CONCEPT config_element = std::semiregular<std::remove_reference_t<config_t>> &&


can you make a new commit and rename this to config_element_specialisation. We decided to call concepts that repesent a type specialisation so where we would ask for is_type_specialisation_of with the suffix _specialisation To avoid clashes with the actual type defintion.

rrahn

Looks good. Thank you.

joergi-w requested review from a team and eseiler and removed request for a team June 3, 2020 16:48

eseiler requested changes Jun 3, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

include/seqan3/core/algorithm/concept.hpp Outdated Show resolved Hide resolved

include/seqan3/core/algorithm/concept.hpp Outdated Show resolved Hide resolved

marehr reviewed Jun 4, 2020

View reviewed changes

include/seqan3/core/algorithm/configuration.hpp Outdated Show resolved Hide resolved

marehr mentioned this pull request Jun 7, 2020

[FEATURE] Add align_cfg::on_result configuration element. #1876

Merged

marehr mentioned this pull request Jun 8, 2020

pipeable_config_element disallow construction of base class seqan/product_backlog#118

Closed

marehr previously requested changes Jun 8, 2020

View reviewed changes

include/seqan3/core/algorithm/pipeable_config_element.hpp Outdated Show resolved Hide resolved

include/seqan3/core/algorithm/pipeable_config_element.hpp Outdated Show resolved Hide resolved

joergi-w requested a review from eseiler June 9, 2020 13:37

joergi-w force-pushed the config_values branch from 53a1cfb to 5a6aa73 Compare June 9, 2020 14:04

[MISC] Create a version of pipeable_config_element without value.

1a25dfb

joergi-w force-pushed the config_values branch from 5a6aa73 to 1a25dfb Compare June 9, 2020 14:05

eseiler approved these changes Jun 9, 2020

View reviewed changes

marehr mentioned this pull request Jun 9, 2020

[API] The alignment configuration for the band changed to seqan3::align_cfg::band_fixed_size #1873

Merged

marehr requested review from a team and rrahn and removed request for a team June 9, 2020 20:48

[FIX] documentation link error

8b6c114

joergi-w changed the title ~~[API] Create a version of pipeable_config_element without value~~ Create a version of pipeable_config_element without value Jun 10, 2020

rrahn requested changes Jun 10, 2020

View reviewed changes

[API] rename config_element to config_element_specialisation

915c0c9

joergi-w requested a review from rrahn June 10, 2020 16:36

rrahn approved these changes Jun 10, 2020

View reviewed changes

rrahn merged commit 2248404 into seqan:master Jun 10, 2020

joergi-w deleted the config_values branch June 16, 2020 12:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a version of pipeable_config_element without value #1871

Create a version of pipeable_config_element without value #1871

joergi-w commented Jun 3, 2020

eseiler left a comment

marehr commented Jun 4, 2020 •

edited

Loading

marehr commented Jun 7, 2020

joergi-w commented Jun 8, 2020

marehr commented Jun 8, 2020

marehr left a comment •

edited

Loading

joergi-w commented Jun 9, 2020

eseiler left a comment

marehr commented Jun 9, 2020 •

edited

Loading

eseiler commented Jun 9, 2020 •

edited

Loading

joergi-w commented Jun 9, 2020

joergi-w commented Jun 10, 2020

eseiler commented Jun 10, 2020

rrahn left a comment

rrahn Jun 10, 2020

joergi-w Jun 10, 2020

rrahn Jun 10, 2020

rrahn Jun 10, 2020

joergi-w Jun 10, 2020

rrahn left a comment

	* \returns The position of the first type `t` in `pack_t` for whom ``pred_t<t>::%value`` is true.
	* \returns The position of the first type `t` in `pack_t` for whom `pred_t<t>\::value` is true.

Create a version of pipeable_config_element without value #1871

Create a version of pipeable_config_element without value #1871

Conversation

joergi-w commented Jun 3, 2020

eseiler left a comment

Choose a reason for hiding this comment

marehr commented Jun 4, 2020 • edited Loading

marehr commented Jun 7, 2020

joergi-w commented Jun 8, 2020

marehr commented Jun 8, 2020

marehr left a comment • edited Loading

Choose a reason for hiding this comment

joergi-w commented Jun 9, 2020

eseiler left a comment

Choose a reason for hiding this comment

marehr commented Jun 9, 2020 • edited Loading

eseiler commented Jun 9, 2020 • edited Loading

joergi-w commented Jun 9, 2020

joergi-w commented Jun 10, 2020

eseiler commented Jun 10, 2020

rrahn left a comment

Choose a reason for hiding this comment

rrahn Jun 10, 2020

Choose a reason for hiding this comment

joergi-w Jun 10, 2020

Choose a reason for hiding this comment

rrahn Jun 10, 2020

Choose a reason for hiding this comment

rrahn Jun 10, 2020

Choose a reason for hiding this comment

joergi-w Jun 10, 2020

Choose a reason for hiding this comment

rrahn left a comment

Choose a reason for hiding this comment

marehr commented Jun 4, 2020 •

edited

Loading

marehr left a comment •

edited

Loading

marehr commented Jun 9, 2020 •

edited

Loading

eseiler commented Jun 9, 2020 •

edited

Loading