Add a copy function for span as mentioned in issue #248 #344

MikeGitb · 2016-08-29T12:24:10Z

Update: The original eiscription is completely outdated -- see the following posts.

Original:
This PR implements a copy function for span similar to what was mentioned in issue #248. However, instead of void, it returns a span representing the remaining space in the destination span.

There are probably a lot of style issues to be addressed and I'd be happy to incorporate any feedback, but my main question at the moment would be if the design is OK and if that function should be put into the span header as I did or into some separate utility / algorithms header.

msftclas · 2016-08-29T12:24:14Z

Hi @MikeGitb, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

msftclas · 2016-09-17T08:52:37Z

@MikeGitb, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

MikeGitb · 2016-09-17T09:20:08Z

I put the function and tests into separate files for now to prevent re-basing conflicts in the future. I'd be happy to move the contents into existing files if that is preferred.

MikeGitb · 2016-09-23T07:12:25Z

Upon revisiting this PR I realized that returning the remaining destination space is maybe not the most intuitive semantics. It was born from from my primary use case to successively fill a buffer, but that might not be such a common use case in the wild.

Possible alternatives:

Return a span over the copied elements (the inverse from what it returns now).
Return the number of copied elements (which is always src.size()).
Don't return anything for now.

Any thoughts?

MikeGitb · 2016-09-24T21:25:33Z

After seeing this version again in @BjarneStroustrup's keynote slides, I settled - at least for this PR - for the simple version that returns nothing.

The question if and what the function should return in addition can then be discussed separately.

neilmacintosh

Thanks for putting this pack together @MikeGitb! Sorry to take so long to get to reviewing it!

I'm still not 100% sure we shouldn't match std::copy_n and return a span that contains the "remaining" area of the destination span. I'll go back and reread your thinking on that and try to form an opinion.

neilmacintosh · 2016-11-04T21:51:46Z

gsl/gsl_algorithm

+#pragma warning(pop)
+#endif
+
+#endif // GSL_SPAN_H


Nit: I think this should be GSL_ALGORITHM_H

neilmacintosh · 2016-11-04T21:52:34Z

gsl/gsl_algorithm

+
+// turn off some misguided warnings
+#pragma warning(push)
+#pragma warning(disable : 4351) // warns about newly introduced aggregate initializer behavior


I'm not sure either of these would fire for this header, so you should be able to remove the whole push/pop sequence.

neilmacintosh · 2016-11-04T21:53:30Z

gsl/gsl_algorithm

+#define noexcept /*noexcept*/
+#endif
+
+#pragma push_macro("alignof")


I couldn't see alignof being used (maybe I missed it). But if it is not used, we can get rid of this push/pop sequence.

neilmacintosh · 2016-11-04T22:06:44Z

gsl/gsl_algorithm

+
+template<class SrcElementType,  std::ptrdiff_t SrcExtent,
+         class DestElementType, std::ptrdiff_t DestExtent>
+void copy(const span<SrcElementType, SrcExtent> src, span<DestElementType, DestExtent> dest)


The const on the first span is unnecessary and probably misleading if passing by-value (as you should rightly do here).

neilmacintosh · 2016-11-04T22:07:25Z

gsl/gsl_algorithm

+#ifdef GSL_THROW_ON_CONTRACT_VIOLATION
+
+#ifdef _MSC_VER
+#pragma push_macro("noexcept")


Given there's no noexcept in this file, I think you can get rid of this push/pop sequence too.

neilmacintosh · 2016-11-04T22:07:58Z

gsl/gsl_algorithm

+// VS 2013 workarounds
+#if _MSC_VER <= 1800
+
+#define GSL_MSVC_HAS_VARIADIC_CTOR_BUG


None of these define's should be necessary for this file I hope.

neilmacintosh · 2016-11-04T22:09:40Z

gsl/gsl_algorithm

+#pragma warning(disable : 26481 26482 26483 26485 26490 26491 26492 26493 26495)
+
+// No MSVC does constexpr fully yet
+#pragma push_macro("constexpr")


no use of constexpr here so this block and its corresponding "pop" can go.

neilmacintosh · 2016-11-05T00:04:25Z

tests/algorithm_tests.cpp

+        }
+    }
+
+    TEST(copy_span_static_diff_type)


This is for a type that is assignable. Could we have a test that validates that the reverse assignment (for example) does not compile? Similarly, we should probably validate that you can't accidentally slice objects using this function.

Should slicing really be prevented? Although it is rarely necessary, I actually like the fact, that std::copy / std::copy_n allow copying from one type to another. If that should not be the case, then we probably should make input and output type identical (modulo constness and maybe volatile)

MikeGitb · 2016-11-05T00:38:47Z

Thanks for the feedback. The header and footer where dump copy past from another file and didn't clean it up afterwards. I'll remove the things you mentioned and expand the tests.

MikeGitb · 2016-11-12T14:47:00Z

@neilmacintosh: I adressed some of your points, but not all:

Regarding the return type: Personally I'd also prefer to return the remaining target range, I just followed the specification presented by @BjarneStroustrup. The other thing is that the iterator returned by std::copy_n can be viewed both as the end of the copied elements or the start of remaining range. For gsl::copy, we have to make a decision for one ot the other.
In any case, I think this can be discussed in a separete issue/PR. Adding a return value is not a big problem
The question about slicing and narrowing conversion: I'm following the convention by the stl, but we can make it more conservative e.g. by
- requiring that source and destination span have the same element types
- Allowing different types, but but not allowing narrowing conversions

neilmacintosh · 2016-11-17T06:31:05Z

@MikeGitb I think you are correct to follow the std::copy_n convention here regarding the slicing and narrowing conversions. It's not perfect, but it will be unsurprising to follow existing standard library practice there.

I had a think about the return value, and although Bjarnes slides demonstrate a void return type, I think that - again - it would be good to stick with the conventions set by the standard library (except in cases where we think they are bad practice). In this case, it seems relatively easy to return aspanwhich runs from one past the last-written element indesttodest.end(). If there are no unwritten elements remaining indest`, then you would just return an empty span. I think this also supports use cases like your original one - where you want to progressively fill a buffer.

However, I can understand if you'd like to take a second PR to add the return type. This has been sitting around for a while ;-)

Let me know your preference...

MikeGitb · 2016-11-17T10:59:06Z

@neilmacintosh: As the decision about the return type is made it doesn't really matter for me, but if you are fine with the rest, I'd prefer if you merge the PR as it is. Adding the return type is simple, but in case the code and/or unittest changes introduce any issues, I'd like to discuss/fix them separately, as I only work on this very irregularly.

neilmacintosh · 2016-11-17T18:44:35Z

Makes sense @MikeGitb. I'm happy with everything in this PR, and agree we can always change the return type separately. Thanks for taking on this work!

msftclas added the cla-required label Aug 29, 2016

msftclas added cla-signed and removed cla-required labels Sep 17, 2016

MikeGitb force-pushed the add_copy branch from 04f9105 to e3fe135 Compare September 17, 2016 09:14

martinmoene mentioned this pull request Sep 20, 2016

Add a copy function for span as mentioned in M-GSL issue #248, PR #344 gsl-lite/gsl-lite#36

Closed

MikeGitb force-pushed the add_copy branch 2 times, most recently from 62f15e3 to 05032e7 Compare September 21, 2016 21:37

MikeGitb changed the title ~~Add a copy function for span as mentioned in issu #248~~ Add a copy function for span as mentioned in issue #248 Sep 21, 2016

neilmacintosh self-assigned this Sep 22, 2016

neilmacintosh added the open label Sep 22, 2016

MikeGitb force-pushed the add_copy branch from 05032e7 to 9bfe3cb Compare September 24, 2016 21:17

neilmacintosh requested changes Nov 5, 2016

View reviewed changes

neilmacintosh added review and removed open labels Nov 5, 2016

MikeGitb force-pushed the add_copy branch from 9bfe3cb to 045748a Compare November 12, 2016 14:23

neilmacintosh approved these changes Nov 17, 2016

View reviewed changes

Add a copy function for span

c9407bf

MikeGitb force-pushed the add_copy branch from 045748a to c9407bf Compare November 17, 2016 10:34

MikeGitb closed this Nov 17, 2016

MikeGitb reopened this Nov 17, 2016

msftclas added the cla-already-signed label Nov 17, 2016

neilmacintosh merged commit d2dafe2 into microsoft:master Nov 17, 2016

neilmacintosh mentioned this pull request Nov 17, 2016

std::copy doesn't work on 2 span #248

Closed

MikeGitb mentioned this pull request Nov 19, 2016

gsl::copy returns remaining range #422

Closed

MikeGitb deleted the add_copy branch November 19, 2016 01:48

dmitrykobets-msft mentioned this pull request Jul 25, 2023

gsl::copy is not in the Core Guidelines isocpp/CppCoreGuidelines#2111

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a copy function for span as mentioned in issue #248 #344

Add a copy function for span as mentioned in issue #248 #344

MikeGitb commented Aug 29, 2016 •

edited

Loading

msftclas commented Aug 29, 2016

msftclas commented Sep 17, 2016

MikeGitb commented Sep 17, 2016

MikeGitb commented Sep 23, 2016

MikeGitb commented Sep 24, 2016 •

edited

Loading

neilmacintosh left a comment

neilmacintosh Nov 4, 2016

neilmacintosh Nov 4, 2016

neilmacintosh Nov 4, 2016

neilmacintosh Nov 4, 2016

neilmacintosh Nov 4, 2016

neilmacintosh Nov 4, 2016

neilmacintosh Nov 4, 2016

neilmacintosh Nov 5, 2016

MikeGitb Nov 5, 2016

MikeGitb commented Nov 5, 2016

MikeGitb commented Nov 12, 2016 •

edited

Loading

neilmacintosh commented Nov 17, 2016

MikeGitb commented Nov 17, 2016

neilmacintosh commented Nov 17, 2016

Add a copy function for span as mentioned in issue #248 #344

Add a copy function for span as mentioned in issue #248 #344

Conversation

MikeGitb commented Aug 29, 2016 • edited Loading

msftclas commented Aug 29, 2016

msftclas commented Sep 17, 2016

MikeGitb commented Sep 17, 2016

MikeGitb commented Sep 23, 2016

MikeGitb commented Sep 24, 2016 • edited Loading

neilmacintosh 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

MikeGitb commented Nov 5, 2016

MikeGitb commented Nov 12, 2016 • edited Loading

neilmacintosh commented Nov 17, 2016

MikeGitb commented Nov 17, 2016

neilmacintosh commented Nov 17, 2016

MikeGitb commented Aug 29, 2016 •

edited

Loading

MikeGitb commented Sep 24, 2016 •

edited

Loading

MikeGitb commented Nov 12, 2016 •

edited

Loading