[FEATURE] Add seqan3::copyable_wrapper #2963

eseiler · 2022-04-12T12:51:31Z

This replaces seqan3::semiregular_box (implemented via range-v3) with seqan3::copyable_wrapper (independent of range-v3).

In early C++20 days, views were required to be default_initialisable (i.e., view{}).
For views that store a lambda, like filter or transform, this meant that the lambda would need to be default_constructible. To do this, semiregular_box provided a wrapper that made any type semiregular (copyable + default_constructible).

After some amendments to C++20, views are conditionally default_constructible (it is not a requirement anymore). Hence, we only need to make a lambda copyable -> copyable_wrapper.
However, this change only applies starting with GCC12, for earlier versions, we still need semiregular (SEQAN3_WORKAROUND_DEFAULT_CONSTRUCTIBLE_VIEW takes care of this).

There are two specialisations:

1 as defined in https://eel.is/c++draft/range.copy.wrap#1, is basically a std::optional with some extras to model copyable
2 as defined in https://eel.is/c++draft/range.copy.wrap#2, just holds a copy of whatever type we need, it's a simplification of 1, and requires less stack memory, etc.

Note:

SEQAN3_WORKAROUND_DEFAULT_CONSTRUCTIBLE_VIEW enables another check for 2. We can only use 2 with gcc<12 iff the underlying type is default_constructible. Otherwise, we need to fall back to 1 - std::optional offers default_constructible.

References:

vercel · 2022-04-12T12:51:36Z

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/47y3BFmLSXaH92nNpcnVYwxE7eon
✅ Preview: https://seqan3-git-fork-eseiler-misc-range-seqan.vercel.app

codecov · 2022-04-14T09:41:59Z

Codecov Report

Merging #2963 (60789bd) into master (f10e557) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2963   +/-   ##
=======================================
  Coverage   98.19%   98.19%           
=======================================
  Files         267      268    +1     
  Lines       11521    11525    +4     
=======================================
+ Hits        11313    11317    +4     
  Misses        208      208

Impacted Files	Coverage Δ
...alignment/configuration/align_config_on_result.hpp	`100.00% <100.00%> (ø)`
include/seqan3/core/detail/copyable_wrapper.hpp	`100.00% <100.00%> (ø)`
include/seqan3/io/views/detail/take_until_view.hpp	`96.42% <100.00%> (-0.13%)`	⬇️
include/seqan3/search/configuration/on_result.hpp	`100.00% <100.00%> (ø)`
...qan3/alignment/pairwise/alignment_configurator.hpp	`97.91% <0.00%> (-0.05%)`	⬇️
...nclude/seqan3/core/configuration/configuration.hpp	`100.00% <0.00%> (ø)`

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 f10e557...60789bd. Read the comment docs.

vercel · 2022-04-22T11:27:49Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
seqan3	✅ Ready (Inspect)	Visit Preview	May 17, 2022 at 11:12AM (UTC)

include/seqan3/core/copyable_wrapper.hpp

include/seqan3/io/views/detail/take_until_view.hpp

include/seqan3/core/copyable_wrapper.hpp

smehringer

Only questions. I don't know if I fully understood every change you made.

smehringer · 2022-05-12T17:03:27Z

include/seqan3/core/copyable_wrapper.hpp

+/*!\brief Helper concept for seqan3::copyable_wrapper.
+ * \ingroup core
+ * \see https://en.cppreference.com/w/cpp/ranges/copyable_wrapper
+ * \noapi{Exposition only.}


why can't this be in seqan3::detail?

I did not because semiregular_box wasn't...but because it's core, we can just move it to detail, right?

Exposition only. only refers to the standard, where it is considered as such. Won't matter if we move it to detail

Shall I move it into detail?

I missed that in my former re-review.

IMHO, I would avoid "exposition only" mentions since we already have the detail namespace and the \noapi tag for stuff that we don't give guarantees for. Because this doesn't seem to be something we will expose to the user at any given point, I see no reason to NOT make it detail?

smehringer · 2022-05-12T17:08:05Z

include/seqan3/core/copyable_wrapper.hpp

+     * This is a SeqAn-specific extension that allows easy invocation of the wrapped object.
+     * `t` needs to be callable with the passed arguments.
+     *


Can you give a short example?

seqan3::copyable_wrapper wrapper{[] (int const x) { return x + 1; }}; auto wrapper_2 = wrapper; // A lambda function wouldn't be copyable. wrapper(2); // Returns 3 wrapper_2(2); // Returns 3

It's a functionality that semiregular_box had, but is not part of the Standard.
And we use it quite heavily, the alternative would be wrapper->operator()(2).

Suggested change

* This is a SeqAn-specific extension that allows easy invocation of the wrapped object.

* `t` needs to be callable with the passed arguments.

*

* This is a SeqAn-specific extension that allows easy invocation of the wrapped object.

* `t` needs to be callable with the passed arguments.

* e.g.:

* ```

* seqan3::copyable_wrapper wrapper{[] (int const x) { return x + 1; }};

* auto wrapper_2 = wrapper; // A lambda function wouldn't be copyable.

* wrapper(2); // Returns 3

* wrapper_2(2); // Returns 3

* ```

*

since this is deep detail I'd be fine with no snippet but hard coded code

smehringer · 2022-05-12T17:09:05Z

include/seqan3/core/copyable_wrapper.hpp

+ * \see https://en.cppreference.com/w/cpp/ranges/copyable_wrapper
+ * \details
+ *
+ * If `t` is `std::copyable`, the STL allows to only store `t`, i.e. no `std::optional` is needed.


Suggested change

* If `t` is `std::copyable`, the STL allows to only store `t`, i.e. no `std::optional` is needed.

* If `t` is `std::copyable`, the STL allows to store `t` directly, i.e. no `std::optional` is needed.

Will apply when applying other changes.

include/seqan3/io/views/detail/take_until_view.hpp

test/snippet/alphabet/views/trim_quality_phred42.cpp

smehringer

The two doc issues but than it LGTM

smehringer · 2022-05-16T13:08:45Z

include/seqan3/core/copyable_wrapper.hpp

+     * This is a SeqAn-specific extension that allows easy invocation of the wrapped object.
+     * `t` needs to be callable with the passed arguments.
+     *


Suggested change

* This is a SeqAn-specific extension that allows easy invocation of the wrapped object.

* `t` needs to be callable with the passed arguments.

*

* This is a SeqAn-specific extension that allows easy invocation of the wrapped object.

* `t` needs to be callable with the passed arguments.

* e.g.:

* ```

* seqan3::copyable_wrapper wrapper{[] (int const x) { return x + 1; }};

* auto wrapper_2 = wrapper; // A lambda function wouldn't be copyable.

* wrapper(2); // Returns 3

* wrapper_2(2); // Returns 3

* ```

*

since this is deep detail I'd be fine with no snippet but hard coded code

smehringer · 2022-05-16T19:00:14Z

include/seqan3/core/copyable_wrapper.hpp

+/*!\brief Helper concept for seqan3::copyable_wrapper.
+ * \ingroup core
+ * \see https://en.cppreference.com/w/cpp/ranges/copyable_wrapper
+ * \noapi{Exposition only.}


I missed that in my former re-review.

IMHO, I would avoid "exposition only" mentions since we already have the detail namespace and the \noapi tag for stuff that we don't give guarantees for. Because this doesn't seem to be something we will expose to the user at any given point, I see no reason to NOT make it detail?

eseiler · 2022-05-17T10:48:23Z

@smehringer I did add a snippet. As it turns out, it's really, really hard to deliberately make a lambda non-copyable in cpp20. So, the example shows how to use operator(), but the lambda itself would already be copyable by default.
Non-copyable lambdas happen in the wild (e.g., alignment-module) though, just seems way more involved and hard to show in a small snippet.

eseiler force-pushed the misc/range branch from 91dd62b to 4dfc616 Compare April 12, 2022 13:26

eseiler force-pushed the misc/range branch from 971b574 to ed5f9ba Compare April 14, 2022 13:57

eseiler marked this pull request as ready for review April 22, 2022 11:24

eseiler force-pushed the misc/range branch from ed5f9ba to f2c97ef Compare April 22, 2022 11:27

eseiler mentioned this pull request Apr 22, 2022

A Life Without range-v3 seqan/product_backlog#124

Closed

eseiler commented Apr 22, 2022

View reviewed changes

include/seqan3/core/copyable_wrapper.hpp Outdated Show resolved Hide resolved

include/seqan3/core/copyable_wrapper.hpp Outdated Show resolved Hide resolved

include/seqan3/core/copyable_wrapper.hpp Outdated Show resolved Hide resolved

eseiler requested review from a team and SGSSGene and removed request for a team April 25, 2022 11:45

eseiler force-pushed the misc/range branch from f2c97ef to 52f1790 Compare April 25, 2022 12:05

SGSSGene requested changes Apr 26, 2022

View reviewed changes

include/seqan3/io/views/detail/take_until_view.hpp Show resolved Hide resolved

include/seqan3/core/copyable_wrapper.hpp Outdated Show resolved Hide resolved

include/seqan3/core/copyable_wrapper.hpp Outdated Show resolved Hide resolved

SGSSGene approved these changes May 2, 2022

View reviewed changes

SGSSGene requested review from a team and smehringer and removed request for a team May 2, 2022 08:42

eseiler force-pushed the misc/range branch from 52f1790 to 1b4982b Compare May 3, 2022 11:37

smehringer reviewed May 12, 2022

View reviewed changes

eseiler requested a review from smehringer May 12, 2022 17:30

eseiler mentioned this pull request May 13, 2022

[TEST] add concept checks for deep views::char_to #2980

Closed

smehringer approved these changes May 16, 2022

View reviewed changes

smehringer requested changes May 16, 2022

View reviewed changes

eseiler force-pushed the misc/range branch 2 times, most recently from f09c6b7 to 6ccd4cf Compare May 17, 2022 10:45

eseiler requested a review from smehringer May 17, 2022 10:46

smehringer approved these changes May 17, 2022

View reviewed changes

[FEATURE] Add seqan3::copyable_wrapper

9a04493

eseiler force-pushed the misc/range branch from 6ccd4cf to b59c32b Compare May 17, 2022 10:54

eseiler enabled auto-merge May 17, 2022 10:54

eseiler added 2 commits May 17, 2022 13:06

[MISC] Use seqan3::copyable_wrapper instead of semiregular_box

3e84d62

[FIX,TEST] Unused variable warnings in snippets

60789bd

eseiler force-pushed the misc/range branch from b59c32b to 60789bd Compare May 17, 2022 11:06

eseiler merged commit 369ae97 into seqan:master May 17, 2022

eseiler deleted the misc/range branch May 17, 2022 11:41

eseiler restored the misc/range branch May 17, 2022 13:05

eseiler deleted the misc/range branch May 17, 2022 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] Add seqan3::copyable_wrapper #2963

[FEATURE] Add seqan3::copyable_wrapper #2963

eseiler commented Apr 12, 2022 •

edited

Loading

vercel bot commented Apr 12, 2022 •

edited

Loading

codecov bot commented Apr 14, 2022 •

edited

Loading

vercel bot commented Apr 22, 2022 •

edited

Loading

smehringer left a comment

smehringer May 12, 2022

eseiler May 12, 2022 •

edited

Loading

smehringer May 16, 2022

smehringer May 12, 2022

eseiler May 12, 2022

smehringer May 16, 2022

smehringer May 12, 2022

eseiler May 12, 2022

smehringer left a comment

smehringer May 16, 2022

smehringer May 16, 2022

eseiler commented May 17, 2022

-     * This is a SeqAn-specific extension that allows easy invocation of the wrapped object.
-     * `t` needs to be callable with the passed arguments.
-     *
+     * This is a SeqAn-specific extension that allows easy invocation of the wrapped object.
+     * `t` needs to be callable with the passed arguments.
+     * e.g.:
+     * ```
+     * seqan3::copyable_wrapper wrapper{[] (int const x) { return x + 1; }};
+     * auto wrapper_2 = wrapper; // A lambda function wouldn't be copyable.
+     * wrapper(2); // Returns 3
+     * wrapper_2(2); // Returns 3
+     * ```
+     *
+since this is deep detail I'd be fine with no snippet but hard coded code

	* If `t` is `std::copyable`, the STL allows to only store `t`, i.e. no `std::optional` is needed.
	* If `t` is `std::copyable`, the STL allows to store `t` directly, i.e. no `std::optional` is needed.

[FEATURE] Add seqan3::copyable_wrapper #2963

[FEATURE] Add seqan3::copyable_wrapper #2963

Conversation

eseiler commented Apr 12, 2022 • edited Loading

vercel bot commented Apr 12, 2022 • edited Loading

codecov bot commented Apr 14, 2022 • edited Loading

Codecov Report

vercel bot commented Apr 22, 2022 • edited Loading

smehringer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eseiler May 12, 2022 • edited Loading

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

smehringer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eseiler commented May 17, 2022

eseiler commented Apr 12, 2022 •

edited

Loading

vercel bot commented Apr 12, 2022 •

edited

Loading

codecov bot commented Apr 14, 2022 •

edited

Loading

vercel bot commented Apr 22, 2022 •

edited

Loading

eseiler May 12, 2022 •

edited

Loading