[FEATURE] Adds all_of traits for type list and parameter packs. #1214

rrahn · 2019-08-15T11:53:32Z

Allows to reduce a type pack or type list by checking a predicate on the type identiry of each type.

eseiler · 2019-08-15T12:16:12Z

Without looking too much into it, it seems similar to std::conjunction but more flexible.

h-2

I have some general design concerns, maybe also because I have more of the type_list stuff laid out in my head than is written in code already.

In general I had planned:

pack_traits::apply (takes a transformation trait, applies to every element) [in #1215 ]
pack_traits::apply_v (takes a type trait, applies to every element) [naming not clear yet]

Other names for these could be pack_traits::transform. Or one could use that for something else.

What your proposal does is in fact running an arbitrary function on some values, with the exception that those values are previously constructed from types. With the purpose of later passing a lambda that also actually doesn't to anything other than evaluating a type property.
This is switching between type-space and value-space quite often which is not easy to understand and likely also not the best thing for compiler performance (although I am not sure about it).

I see that we might want the ability to run lambdas on a type list, but I would prefer if we can discuss the general design before blowing it up.

Can you do one of the simpler temporary solutions for #863 like defining a concept and immediately expanding on that?

h-2 · 2019-08-15T12:31:24Z

include/seqan3/core/type_list/traits.hpp

+{
+    return false;
+}
+//!\endcond


Currently the design is that traits should either SFINAE or fail if instantiating them fails. Adding another possibility of "silently working" make this more complicated.

Not sure, what you mean but if I have something that does not SFINAE on an instantiation failure and is still ill-formed it will fail. It's only that you can use in addition SFINAE and do not provide a default case for every trait to test.

include/seqan3/core/type_list/traits.hpp

codecov · 2019-08-15T13:46:52Z

Codecov Report

❗ No coverage uploaded for pull request base (master@ebd0790). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #1214   +/-   ##
=========================================
  Coverage          ?   97.59%           
=========================================
  Files             ?      220           
  Lines             ?     8956           
  Branches          ?        0           
=========================================
  Hits              ?     8741           
  Misses            ?      215           
  Partials          ?        0

Impacted Files	Coverage Δ
include/seqan3/io/sequence_file/output.hpp	`100% <ø> (ø)`
include/seqan3/io/sequence_file/input.hpp	`100% <ø> (ø)`
include/seqan3/core/simd/view_to_simd.hpp	`90.47% <ø> (ø)`
include/seqan3/io/detail/misc.hpp	`100% <100%> (ø)`
include/seqan3/core/detail/pack_algorithm.hpp	`100% <100%> (ø)`
...gnment/configuration/align_config_aligned_ends.hpp	`100% <100%> (ø)`
include/seqan3/search/configuration/max_error.hpp	`100% <100%> (ø)`
...ude/seqan3/search/configuration/max_error_rate.hpp	`90% <100%> (ø)`

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 ebd0790...225023f. Read the comment docs.

rrahn · 2019-08-15T16:14:03Z

In general I had planned:

pack_traits::apply (takes a transformation trait, applies to every element) [in #1215 ]
pack_traits::apply_v (takes a type trait, applies to every element) [naming not clear yet]

Other names for these could be pack_traits::transform. Or one could use that for something else.

Besides that apply means to invoke a function on all arguments and not element wise, we will likely add more algorithms on types because they are useful in many places. At least that is what I had planned.

What your proposal does is in fact running an arbitrary function on some values, with the exception that those values are previously constructed from types. With the purpose of later passing a lambda that also actually doesn't to anything other than evaluating a type property.
This is switching between type-space and value-space quite often which is not easy to understand and likely also not the best thing for compiler performance (although I am not sure about it).

Well, I can't be sure either but it looks like that the compiler will not not have problems optimizing this. Anyway, if performance is your biggest concern we need to use a third library from people that have spent a good time optimising this.

In general, this is so much easier than the pure type based approach and closer to modern c++. You don't need to pollute the namespace with helper structs as you can directly call it in the compile time expression with a lambda. Once we have template names it will become even more easier with lambdas because you don't need the decltype anymore. It is a generic solution that can be used in type and value land.

I see that we might want the ability to run lambdas on a type list, but I would prefer if we can discuss the general design before blowing it up.

Absolutely. Never meant to be the absolute and final design. It is a first proposal that in my opinion is close to the use of standard algorithms and therefor has a major advantage over designs where I need to provide template member types in structs that evaluate to bool_constant types based on SFINAE or constraints.

Can you do one of the simpler temporary solutions for #863 like defining a concept and immediately expanding on that?

Sure.

rrahn · 2019-08-21T11:18:08Z

So I moved the functionality to core/detail/pack_algorithm and also moved the already existing for_each functionality to this file.

rrahn · 2019-08-27T15:43:43Z

@smehringer ping

smehringer · 2019-08-28T08:35:55Z

Travis was failing so I did not review this before

rrahn · 2019-08-28T08:37:41Z

you can ignore jenkins for this PR.

smehringer

You introduce code in commit 1 & 2 that you delete again in commit 3. I guess you plan to squash this or ? It if confusing to review commit by commit because it is hard to track if there are other changes than those of altering your feature again. It would be nice if you split these changes from the feature.

I did not review type traits that often so I am a littel confused about the naming.
When do we call something _type? (all_of_type)

include/seqan3/core/type_list/traits.hpp

test/unit/core/type_list_test.cpp

test/snippet/core/detail/all_of_pack.cpp

test/snippet/core/detail/all_of_type_list.cpp

test/snippet/core/detail/all_of_type_list_2.cpp

smehringer · 2019-08-28T09:50:15Z

test/snippet/core/detail/for_each_type_list.cpp

@@ -30,27 +30,27 @@ int main()
        static_assert(std::is_same_v<id_t, std::type_identity<type>>, "id is of type std::type_identity<type>");

        if constexpr(std::is_same_v<type, bool>)
-            debug_stream << "bool";
+            seqan3::debug_stream << "bool";


remove using namespace seqan3; in line 8

smehringer · 2019-08-28T09:51:30Z

test/snippet/core/detail/for_each_type_list_2.cpp

@@ -0,0 +1,54 @@
+#include <iostream>


you are using debug stream, do you need this include?

rrahn · 2019-08-28T12:23:07Z

You introduce code in commit 1 & 2 that you delete again in commit 3. I guess you plan to squash this or ? It if confusing to review commit by commit because it is hard to track if there are other changes than those of altering your feature again. It would be nice if you split these changes from the feature.

yes. I'll squash this.

I did not review type traits that often so I am a littel confused about the naming.
When do we call something _type? (all_of_type)

Well, we need to somehow distinguish all_of for type lists and parameter packs since we do not use different namespaces as it is done in the type_list traits at the moment. Accordingly, the _type version denotes the overload for type_lists.

smehringer · 2019-08-28T14:27:56Z

Well, we need to somehow distinguish all_of for type lists and parameter packs since we do not use different namespaces as it is done in the type_list traits at the moment. Accordingly, the _type version denotes the overload for type_lists.

Ugh... But we usually put _type behind names that denote a type using iterator_type = ... . It is confusing that a type trait is now called _type :(
Is this a naming we use throughout the library already or did we recently intrduce/dicuss this and I missed it?

rrahn · 2019-09-04T17:45:29Z

Ugh... But we usually put _type behind names that denote a type using iterator_type = ... . It is confusing that a type trait is now called _type :(
Is this a naming we use throughout the library already or did we recently intrduce/dicuss this and I missed it?

This was already introduced for for_each_type, which was a name used to iterate over types in a type list or template parameter pack. We also had this for parameter packs called for_each_value. I removed the value suffix for the parameter packs, but we need a different name for the type versions otherwise there is an ambiguous overload. Note, that these functions are not purely type_traits and as such are not added to type_list/traits, where we explicitly introduced namespaces. Since this is pure detail and only for developers I guess the naming is not so relevant. In general, I am not a fan of using the suffix type in a class name as well, since all classes are types. We had something like for_each_in_type_list but that was to verbose for the use cases we considered. So if you like you can add a discussion item for the next strategy meeting but my feeling is that it is not worth it to have a lengthy discussion about it.

smehringer · 2019-09-05T05:50:07Z

I agree that if it is strictly detail it's not worth a lengthy discussion.
One more suggestion though, what about type_list_for_each/type_list_all_of? It would mimic the namespace you mentioned for type traits (that I assume would be `type_list::all_of``).
If you disagree, I will just review the PR again.

rrahn · 2019-09-09T14:13:33Z

@smehringer ping

smehringer · 2019-09-09T19:18:39Z

I was waiting for an answer but I guess this means you disagree?

smehringer · 2019-09-10T10:52:12Z

since I am second reviewer now maybe someone else should review this first

rrahn · 2019-09-13T15:33:50Z

@marehr ping

marehr · 2019-09-16T11:39:01Z

@marehr ping

Sorry for the late response, I have the feeling that this PR would be best discussed in person.

rrahn · 2019-09-16T12:15:42Z

Sorry for the late response, I have the feeling that this PR would be best discussed in person.

Ok, then please put it on the card for next Monday discussion. I do agree that we need to be certain about the design and that here are multiple cornerstones here.

marehr · 2019-09-16T13:53:37Z

Yep. I added a card https://github.com/orgs/seqan/projects/4#card-26492576

joshuak94 · 2019-09-30T13:10:13Z

@rrahn What's the status here?

rrahn · 2019-10-01T14:56:15Z

@joshuak94 I am waiting on the review, From POV this is done.

marehr

🤸‍♂️ Thank you.The code looks good to me. Just some smaller and sometimes a tiny bit bigger smaller naming and documentation stuff, as well as proposal to remove tuple support. 🤸‍♀️ Took me longer than 1am xD