[FEATURE] our own meta library #1204

h-2 · 2019-08-07T23:06:51Z

This is our own meta library with almost identical interfaces for dealing with seqan3::type_list, but also parameter packs (it's usually faster to work on the latter).

Currently available:

at (linear-time)
front
back
concat
drop (linear-time)
drop_front (constant)
drop_last (linear-time)
take (linear-time)
take_last (linear-time)
take (linear-time)
split_after (linear-time)

Notably absent:

drop_back because it's not possible to do in constant time without integer_sequence and a whole lot instantiaten. Actually, back was already tricky! @rrahn: You could have a look at this after we merge this, or you could just use drop_front on your example and move the void to the top and the commas to the front.

codecov · 2019-08-08T00:04:14Z

Codecov Report

Merging #1204 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1204   +/-   ##
=======================================
  Coverage   96.72%   96.72%           
=======================================
  Files         210      210           
  Lines        8003     8003           
=======================================
  Hits         7741     7741           
  Misses        262      262

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 b70c68d...8787e1e. Read the comment docs.

rrahn

Looks quite nice. In general if you switch this to another header we might as well get the for_each... functions in core/algorithm/parameter_pack.hpp to the proper headers and namespaces. That would also mean some API breakage so you need to add a Changelog entry as well.

rrahn · 2019-08-08T13:12:05Z

include/seqan3/core/type_list.hpp

@@ -19,6 +19,10 @@
 namespace seqan3
 {

+// ----------------------------------------------------------------------------
+// type_list class
+// ----------------------------------------------------------------------------


we do need a followup issue to remove all meta:: dependencies and replace them with our own. Can you track this somewhere explicit?

Yes, I know.

added a card to Core project

rrahn · 2019-08-08T13:13:21Z

include/seqan3/core/type_list.hpp

+ * \ingroup type_traits
+ */
+template <ptrdiff_t idx, typename head_t, typename ...tail_t>
+auto at_impl()


now, since it is in detail we might not need the impl suffix right?

well they are different things (type vs functions...) so I wasn't sure. But I don't care much about it.

but all impl stuff are now functions, aren't they?

Yes, but the public entity isn't. Anyway, I can change it.

rrahn · 2019-08-08T13:36:09Z

include/seqan3/core/type_list.hpp

+ * Constant.
+ */
+template <typename ...pack_t>
+using concat = type_list<pack_t...>;


that is confusing to me since I expect something different from a concat. Also I know what you mean but you are not concatenating two or more packs. From STL nomenclature this would be more a make_type_list

Honestly, I don't think we need this at all. I mean that would be simply type_list<t1, t2, t3, t4>. Why bother to have a separate traits for this, which is basically just an alias?

Well I wanted to keep the interfaces for list VS types very similar.

You might have

template <typename list1, typename list2>

and want to go to concat<list1, list2>.

Similarly:

template <typename ...pack1, typename ...pack2>

and go to concat<pack1..., pack2...> .

Of course you could just write type_list for the packs, but I think there is value in consistency...

But, there is no consistency with this, because

template <typename ...pack1, typename ...pack2>

won't be possible if they are not already wrapped in some class. Which does not happen to often outside of the context of type_lists anyway. Also since they are in different namespaces you have to explicitly choose the traits anyway. Hence, there is no need for consistency. Either I am in the context of parameter packs or I am in the context of type_lists. Because then you could also argue that you could allow

template <typename ...pack1, typename ...pack2, typename ...pack3> ... concat<pack1..., pack2..., pack3....>

But this does not work with type_lists and would be more confusing then because of inconsistencies. Of course we could blow up the concat for type_lists but I think that it is rarely necessary (ok, let's see about that). So I'd like to keep these things separate and would argue that for parameter packs a concat is simply not necessary.

Not convinced fully, but I will remove, because it's really not that important.

rrahn · 2019-08-08T13:39:23Z

include/seqan3/core/type_list.hpp

+seqan3::pack_traits::drop_front<pack_t...> drop_front_impl(type_list<pack_t...>);
+
+/*!\brief Implementation for take, drop, take_last and drop_last in seqan3::list_traits.
+ * \tparam idx       The after which to split.


Suggested change

* \tparam idx The after which to split.

* \tparam idx The index after which to split.

rrahn · 2019-08-08T13:44:09Z

include/seqan3/core/type_list.hpp

+ * \ingroup type_traits
+ */
+template <ptrdiff_t idx, typename ...pack_t>
+std::type_identity<seqan3::pack_traits::at<idx, pack_t...>> at_impl(type_list<pack_t...>);


same here the impl in the name is kind of redundant given that is is in detail already

I can change it if you want to.

Yes, because otherwise it would deviate from the rest of the code base and kind of counteracts the meaning of the detail namespace.

rrahn · 2019-08-08T14:03:00Z

include/seqan3/core/type_list.hpp

+//!\cond
+    requires (n >= 0 && n <= size<list_t>)
+//!\endcond
+using drop_last = typename decltype(detail::split_after_impl<size<list_t> - n>(type_list<>{}, list_t{}))::first_type;


accordingly we can model this with the negative index and with take as well. I think it is more readable.

I had it like that before, but it's horrible 👻

The problem is that it gets very confusing regarding inclusive/exclusive indexing.

take and drop are specifically not modeled as "take until third element", but as "take three elements". What would take<-3> imply? "Take until the third-last"? Before or after? What does it imply regarding the size of the list?

This way it is very simple: take<n> and take_last <n> always return something of size n. drop<n> and drop_last <n> always return something of the original size - n.

range-v3 is also getting a view::take_last so we would be in-sync regarding naming.

Well, the negative index would perfectly align with take because take<-3> takes exactly the last three elements. That's how it is implemented anyway (take_last<n> <=> drop<N-n>). Thus you would have take<N> == take<-N> and take<1> <=> at<0> and take<-1> <=> at<N-1> <=> at<-1>.
But if the ranges library get's a special take_last then why not having it here as well. But honestly, I don't like that at has then a different semantics. But oh boy, at_last would be the wrong way to go as well 🙈 .

Well, the negative index would perfectly align with take because take<-3> takes exactly the last three elements.

Oh, wow, ok, you thought it would work that way.

But if the ranges library get's a special take_last then why not having it here as well.

Yes, I really think it makes it clearer as we are not working with indexes at all, but with counts. And negative counts are weird.

Oh, wow, ok, you thought it would work that way.

It should work that way if at works that way!

rrahn · 2019-08-08T14:07:55Z

include/seqan3/core/type_list.hpp

+//!\cond
+    requires (n >= 0 && n <= size<list_t>)
+//!\endcond
+using take_last = typename decltype(detail::split_after_impl<size<list_t> - n>(type_list<>{}, list_t{}))::second_type;


First of all this would be the same as:

Suggested change

using take_last = typename decltype(detail::split_after_impl<size<list_t> - n>(type_list<>{}, list_t{}))::second_type;

using take_last = drop<size<list_t> - n>;

right?
And I am wondering if this should not be modeled with negative indicestake<-n, list_t>?
That would it make consistent with at
So:

template <ptrdiff_t n, seqan3::detail::TypeList list_t> //!\cond requires (n < 0 && -n <= size<list_t>) //!\endcond using take = drop<size<list_t> + n>;

I agree, it can be written shorter. See above for why I am opposed to negative idnexing there.

(Believe me, I wanted this, I designed it that way and it confused me so much I got convinced it wasn't a good idea)

rrahn · 2019-08-08T14:08:25Z

test/unit/core/type_list_test.cpp

+
+TEST(pack_traits, size)
+{
+    EXPECT_EQ(( pack_traits::size<int, bool &, double const> ),


Suggested change

EXPECT_EQ(( pack_traits::size<int, bool &, double const> ),

EXPECT_EQ((pack_traits::size<int, bool &, double const>),

rrahn · 2019-08-08T14:09:56Z

test/unit/core/type_list_test.cpp

+
+TEST(pack_traits, concat)
+{
+    EXPECT_TRUE((std::is_same_v<pack_traits::concat<int, bool &, double const, long, float>,


I think we really don't need this. There is no added functionality by this.

It's just an alias 🤷‍♂️

but as I said above an alias that does allow more functionality than it's type list counterpart

h-2 · 2019-08-09T11:45:18Z

I think I have adressed all of your issues. I have also moved the stuff around and added a changelog.

rrahn

lgtm. Only some 💅 stuff

rrahn · 2019-08-12T08:38:42Z

include/seqan3/core/type_list/traits.hpp

+        return std::type_identity<__type_pack_element<idx - 1, tail_t...>>;
+#else
+        return at<idx - 1, tail_t...>();
+#endif


can you mark with a comment that this is builtin ends here.

h-2 requested a review from marehr August 7, 2019 23:06

rrahn requested changes Aug 8, 2019

View reviewed changes

rrahn removed the request for review from marehr August 8, 2019 14:13

h-2 force-pushed the feature/our_meta branch from a2b4292 to 8787e1e Compare August 9, 2019 11:43

h-2 requested a review from rrahn August 9, 2019 13:05

rrahn requested changes Aug 12, 2019

View reviewed changes

h-2 added 3 commits August 12, 2019 15:21

[FEATURE] metaprogramming for type lists and packs

15c3558

[API,INFRA] move type_list.hpp to own subfolder

2a17a46

[DOC] add CHANGELOG

2a4d1d9

h-2 force-pushed the feature/our_meta branch from 8787e1e to 2a4d1d9 Compare August 12, 2019 13:21

h-2 requested a review from rrahn August 12, 2019 13:21

rrahn approved these changes Aug 14, 2019

View reviewed changes

rrahn merged commit 1527991 into seqan:master Aug 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] our own meta library #1204

[FEATURE] our own meta library #1204

h-2 commented Aug 7, 2019 •

edited

Loading

codecov bot commented Aug 8, 2019 •

edited

Loading

rrahn left a comment

rrahn Aug 8, 2019

h-2 Aug 8, 2019

h-2 Aug 12, 2019

rrahn Aug 8, 2019

h-2 Aug 8, 2019

rrahn Aug 9, 2019

h-2 Aug 9, 2019

rrahn Aug 8, 2019

rrahn Aug 8, 2019

h-2 Aug 8, 2019

rrahn Aug 9, 2019

h-2 Aug 9, 2019

rrahn Aug 8, 2019

rrahn Aug 8, 2019

h-2 Aug 8, 2019

rrahn Aug 9, 2019

rrahn Aug 8, 2019

h-2 Aug 8, 2019

rrahn Aug 9, 2019

h-2 Aug 9, 2019

rrahn Aug 14, 2019

rrahn Aug 8, 2019

h-2 Aug 8, 2019

rrahn Aug 8, 2019

rrahn Aug 8, 2019

h-2 Aug 8, 2019

rrahn Aug 9, 2019

h-2 commented Aug 9, 2019

rrahn left a comment

rrahn Aug 12, 2019

	* \tparam idx The after which to split.
	* \tparam idx The index after which to split.

	using take_last = typename decltype(detail::split_after_impl<size<list_t> - n>(type_list<>{}, list_t{}))::second_type;
	using take_last = drop<size<list_t> - n>;

	EXPECT_EQ(( pack_traits::size<int, bool &, double const> ),
	EXPECT_EQ((pack_traits::size<int, bool &, double const>),

[FEATURE] our own meta library #1204

[FEATURE] our own meta library #1204

Conversation

h-2 commented Aug 7, 2019 • edited Loading

codecov bot commented Aug 8, 2019 • edited Loading

Codecov Report

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

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

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 Aug 9, 2019

rrahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-2 commented Aug 7, 2019 •

edited

Loading

codecov bot commented Aug 8, 2019 •

edited

Loading