Add std::unique_ptr support to gsl::span #404

rianquinn · 2016-10-26T18:13:44Z

This patch adds support for std::unique_ptr to the gsl::span
class instead of having to manually grab the pointer from
a std::unique_ptr.

It was suggestd that we also add support for std::shared_ptr,
but no array syntax exists for this class, and the element
types don't match. Until the following is accepted, I suggest
support for std::shared_ptr is left out:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4077.html

For reference, this is part of the following issue:
#402

Signed-off-by: “Rian <“rianquinn@gmail.com”>

neilmacintosh · 2016-10-26T21:16:00Z

@rianquinn I like Jonathan Wakeley's paper, but I don't think there's any need to delay adding support for shared_ptr. A single element is still a contiguous sequence of one. So I would want to support creating a span easily from both unique_ptr and shared-ptr, regardless of whether they contain a single element or an array...given both are commonly used standard library "containers" (even though they don't meet the requirements for a contiguous container).

rianquinn · 2016-10-26T21:20:53Z

That's fine. There a couple of issues (some are just things I have to learn about template programming).

unique_ptr and shared_ptr don't take the same class T. It just so happens that unique_ptr and span are in sync so it work, trying to do the same thing with shared_ptr didn't work as it was complaining about the fact that you would have to define the shared_ptr with T*, but span with T, so I just have to figure out how to strip the reference to make that work (I think... 😄)
The unit test will have some funky code in it to initialize the shared_ptr with some known values. My original though to do that is to actually create a standard array so that I can initialize it, and then pass the pointer to shared_ptr to be managed from there, which in turn would be used to create the span. Obviously all of that could do away once that spec is implemented.

rianquinn · 2016-10-26T22:18:30Z

@neilmacintosh I added support for std::shared_ptr. I actually had it backwards and needed to add a reference to unique_ptr so all around I think this is better.

Sadly, I left my editor in a mode were it removed trailing spaces, which I guess is in issue in the code. If you want, I can re-submit this with the spaces added back in. Up to you.

rianquinn · 2016-10-27T03:53:12Z

I fixed the whitespace issue, so we should be good to go.

rianquinn · 2016-10-28T15:10:30Z

Any other comments on this PR?

neilmacintosh · 2016-10-28T18:46:40Z

I'll take a look a little later on today.

neilmacintosh

Thanks for putting this together @rianquinn! I've left a few comments.

neilmacintosh · 2016-10-28T19:47:29Z

tests/span_tests.cpp

+    {
+        {
+            auto arr = new int[4];
+            auto ptr = std::unique_ptr<int>(arr);


Doesn't this result in the wrong delete being called when ptr goes out of scope?

neilmacintosh · 2016-10-28T19:48:58Z

tests/span_tests.cpp

+                arr[i] = i + 1;
+
+            {
+                span<int> s{arr, 4};


Is arr here supposed to be ptr? I assumed you were trying to test construction of the span from the unique_ptr.

oppsss. nice catch

neilmacintosh · 2016-10-28T19:50:29Z

tests/span_tests.cpp

+            }
+
+            {
+                span<int> s{std::unique_ptr<int>{nullptr}, 0};


While I get that it's probably ok in this exact test case, it seems like it might encourage a misunderstanding to construct a span from a temporary unique_ptr. For any initial value other than nullptr that would result in disaster ;-)

neilmacintosh · 2016-10-28T19:51:27Z

tests/span_tests.cpp

+            }
+
+            {
+                span<int> s{std::unique_ptr<int>{nullptr}, 0};


As per previous comment, container lifetime must always exceed view lifetime.

neilmacintosh · 2016-10-28T19:52:16Z

tests/span_tests.cpp

+        }
+
+        {
+            span<int> s{std::unique_ptr<int>{nullptr}, 0};


I'm assuming this should be shared_ptr, not unique_ptr, but then it should also outlive the span created from it.

neilmacintosh · 2016-10-28T19:52:45Z

tests/span_tests.cpp

+    TEST(from_shared_pointer_construction)
+    {
+        auto arr = new int[4];
+        auto ptr = std::shared_ptr<int>(arr);


I'm pretty sure this will use the wrong delete unless you provide a custom deleter argument.

neilmacintosh · 2016-10-28T19:56:23Z

gsl/span

@@ -386,6 +387,11 @@ public:
    {
    }

+    template<class ArrayElementType = std::add_pointer<element_type>>
+    constexpr span(const std::unique_ptr<ArrayElementType>& ptr, index_type count) : storage_(ptr.get(), count) {}


My feeling is that we want two constructors here, to distinguish between the array and non-array holding versions of unique_ptr'. The non-array version doesn't need a count, it can just be a convenient way to initialize a single-elementspan` from that particular standard container.

neilmacintosh · 2016-10-28T19:57:54Z

gsl/span

+    template<class ArrayElementType = std::add_pointer<element_type>>
+    constexpr span(const std::unique_ptr<ArrayElementType>& ptr, index_type count) : storage_(ptr.get(), count) {}
+
+    constexpr span(const std::shared_ptr<ElementType>& ptr, index_type count) : storage_(ptr.get(), count) {}


Until shared_ptr can officially take multiple elements, I'd be tempted to just have this constructor take a shared_ptr and construct a single-element span from it. If we hear that a lot of people go to the effort of putting dynamically-allocated arrays into shared_ptr, then we can always add the second overload.

This patch adds support for std::unique_ptr and std::shared_ptr to the gsl::span class instead of having to manually grab the pointer via get(). For reference, this is part of the following issue: microsoft#402 Signed-off-by: “Rian <“rianquinn@gmail.com”>

rianquinn · 2016-10-30T16:15:01Z

@neilmacintosh Thanks for the awesome feedback. Sorry it took so long to get the fixes in. I think this patch addresses all of your concerns, and I agree that this is a much cleaner approach as the non-array versions make a lot more sense this way.

neilmacintosh · 2016-11-04T01:55:11Z

Thanks @rianquinn, this is a nice addition to the group of standard library containers supported by span.

msftclas added the cla-already-signed label Oct 26, 2016

neilmacintosh self-assigned this Oct 26, 2016

neilmacintosh added the review label Oct 26, 2016

rianquinn force-pushed the issue_402_1 branch from 967ddb5 to c221450 Compare October 27, 2016 23:30

neilmacintosh requested changes Oct 28, 2016

View reviewed changes

rianquinn force-pushed the issue_402_1 branch from c221450 to dd2d57a Compare October 30, 2016 16:13

martinmoene mentioned this pull request Oct 30, 2016

Add unique_ptr and shared_ptr support to span gsl-lite/gsl-lite#48

Closed

neilmacintosh approved these changes Nov 4, 2016

View reviewed changes

neilmacintosh merged commit 2b51b87 into microsoft:master Nov 4, 2016

neilmacintosh added resolved and removed review labels Nov 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add std::unique_ptr support to gsl::span #404

Add std::unique_ptr support to gsl::span #404

rianquinn commented Oct 26, 2016

neilmacintosh commented Oct 26, 2016

rianquinn commented Oct 26, 2016 •

edited

rianquinn commented Oct 26, 2016

rianquinn commented Oct 27, 2016

rianquinn commented Oct 28, 2016

neilmacintosh commented Oct 28, 2016

neilmacintosh left a comment

neilmacintosh Oct 28, 2016

neilmacintosh Oct 28, 2016

rianquinn Oct 30, 2016

neilmacintosh Oct 28, 2016

neilmacintosh Oct 28, 2016

neilmacintosh Oct 28, 2016

neilmacintosh Oct 28, 2016

neilmacintosh Oct 28, 2016

neilmacintosh Oct 28, 2016

rianquinn commented Oct 30, 2016

neilmacintosh commented Nov 4, 2016

Add std::unique_ptr support to gsl::span #404

Add std::unique_ptr support to gsl::span #404

Conversation

rianquinn commented Oct 26, 2016

neilmacintosh commented Oct 26, 2016

rianquinn commented Oct 26, 2016 • edited

rianquinn commented Oct 26, 2016

rianquinn commented Oct 27, 2016

rianquinn commented Oct 28, 2016

neilmacintosh commented Oct 28, 2016

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

rianquinn commented Oct 30, 2016

neilmacintosh commented Nov 4, 2016

rianquinn commented Oct 26, 2016 •

edited