Platform: Add C++ Span class. #7828

pan- · 2018-08-19T19:33:16Z

Description

The Span class allows the creation of views over contiguous memory. The view
do not own memory, is typed and has a length. It can be used as a replacement of
the traditional pair of pointer and size in parameters or class fields.

Main operations:

size(): return the lenght of the memory viewed
empty(): return if the memory viewed is empty
[index]: access elements viewed
data(): return a pointer to the memory viewed.
first(count): Create a subview from the first count elements.
last(count): Create a subview from the last count elements.
== and !=: compare two views or a view to array and return if they are equal or
not.

The Span class came in two flavors:

Static size: The size is encoded in the Span type and it is as lightweitgh as
a single pointer,
Dynamic size: The object can store arbitrary views and it costs one pointer
and the size of the view.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Breaking change

The Span class allows the creation of views over contiguous memory. The view do not own memory, is typed and has a length. It can be used as a replacement of the traditional pair of pointer and size in parameters or class fields. Main operations: - size(): return the lenght of the memory viewed - empty(): return if the memory viewed is empty - [index]: access elements viewed - data(): return a pointer to the memory viewed. - first(count): Create a subview from the first count elements. - last(count): Create a subview from the last count elements. - == and !=: compare two views or a view to array and return if they are equal or not. The Span class came in two flavors: - Static size: The size is encoded in the Span type and it is as lightweitgh as a single pointer, - Dynamic size: The object can store arbitrary views and it costs one pointer and the size of the view.

pan- · 2018-08-19T19:34:35Z

@donatieng @paul-szczepanek-arm This is an improved version of the Arrayview class we have use in BLE for now quite some time. It is also used by the NFC Ndef layer.

paul-szczepanek-arm · 2018-08-20T07:27:58Z

why not update array view? do we really need both of them?

pan- · 2018-08-20T07:36:43Z

ArrayView is a private API of ble that serves the same purpose. This one is a public API of mbed OS; the plan is to replace the BLE ArrayView by this one if it gets in. Changing the name to Span is more in line with C++ standard; and it is easier to type too.

Two functions have been added: first and last that create subspans. New comparison exists too; it allows programmer to write statement such as:

static const char hello[] = "Hello World!";

bool is_greeting(const Span<uint8_t>& v) { 
    return v == hello;
}

kjbracey

Looks good to me, apart from editing nits. I've been advocating string_view for a while (mainly to client guys) - not sure if this effectively fully covers that, or it would need a few more implicit conversions.

kjbracey · 2018-08-20T08:21:05Z

platform/Span.h

+template<typename T, ptrdiff_t Size = SPAN_DYNAMIC_SIZE>
+struct Span {
+
+    MBED_STATIC_ASSERT(Size >= 0, "Invalid size for an ArrayView");


Search-and-replace fail on the name.

kjbracey · 2018-08-20T08:28:17Z

platform/Span.h

+     */
+    bool empty() const
+    {
+        return size() ? false : true;


Weird implicit boolean inside ternary operator. Surely size() == 0?

kjbracey · 2018-08-20T08:28:53Z

platform/Span.h

+     *
+     * @return The raw pointer to the array.
+     */
+    const T* data() const


Formatting rules say pushing the *& to the right.

kjbracey · 2018-08-20T08:30:15Z

platform/Span.h

+     * @param array_size Number of elements of T present in the array.
+     *
+     * @post a call to size() will return array_size and data() will return
+     * array_tpr.


(@p)? array_ptr

kjbracey · 2018-08-20T09:11:41Z

platform/Span.h

+ * If the type use this value then the size of the array is stored in the object
+ * at runtime.
+ */
+#define SPAN_DYNAMIC_SIZE -1


Just for more naming consistency with C++, should the the template parameter and this constant be "extent" rather than "size"?

Feels a bit wonky, but I guess it's intended to allow you to separately query extent from size in template nonsense. (if (span.extent == std::dynamic_extent) ...)

I will fix the name of the dynamic tag however it is not meant to be queried at runtime as this information is not really useful.

Span with fixed extent are mostly useful for type(def) that requires a fixed size otherwise the dynamic extent should be used. Also, the conversion rules offer implicit conversion from Span<T, Size> to Span<T> so users don't have to worry about conversions.

…deline.

pan- · 2018-08-20T09:59:13Z

Thanks @kjbracey-arm ; I've updated my code according to your review.

To have a string_view we should have a string first! More seriously, even if string functions are not present inside the class, the Span class can already do a lot in SAX type parsers where tokens parsed can be propagated to users without copy.

I think it could be easy to add find functions as free functions targeting Span; all other aspects of string_view are covered by Span.

kjbracey

Few more review notes.

kjbracey · 2018-08-20T09:56:49Z

platform/Span.h

+ * SPAN_DYNAMIC_SIZE  is special as it allows construction of Span objects of
+ * any size (set at runtime).
+ */
+template<typename T, ptrdiff_t Size = SPAN_DYNAMIC_EXTENT>


In C++ the template parameter is also Extent.

kjbracey · 2018-08-20T09:56:53Z

platform/Span.h

+     * @param array_ptr Pointer to the array data
+     * @param array_size Number of elements of T present in the array.
+     *
+     * @post a call to size() will return array_size and data() will return


That's not true - you'll return Size, not array_size.

C++ restricts this constructor to matching size, making the difference only visible in undefined behaviour. Tighten the assert?

I'm not sure to understand why this is still up has it has been fixed in 3985fb8

kjbracey · 2018-08-20T10:11:54Z

platform/Span.h

+     */
+    size_t size() const
+    {
+        return _array ? Size : 0;


Do you really need a static span to have the runtime option of being empty? C++ only permits empty for Extent==0 or dynamic spans. From a compile-time correctness view, I quite like the idea of enforcing non-0 static spans to be non-null.

I think it is valuable to not enforce non-0 static spans to be non-null as it can be use to represent the absence of values. That's useful for optional values represented by a static span.

struct pdu_t { // options have a fixed size typedef Span<uint8_t, 5> option_t; uint8_t header; option_t option; Span<uint8_t> payload; }; void process_pdu(const pdu_t& pdu) { if (!pdu.option.empty()) { // process options ... } }

Note that it raises the question of operator bool() const. It is not part of std::span for consistency with the standard library (see 1 and 2); I believe that is up to debate in our use case; but maybe we can have this debate once this class gets in and have more feedbacks from users.

Ta for links. I can see it's useful, much as allowing references to be null would be useful, but it is a major deviation from the standard C++ equivalent, where that syntax guarantees there are 5 uint8_ts.

I believe the C++ standard guys would tell you to use optional<span>, but then they're not too bothered about doubling memory usage by adding an extra bool alongside a pointer. In the absence of optional, and to save space, maybe you could designate the extended type OptionalSpan to make clear the extended semantics, although it becomes less catchy.

If we ever have an optional type (would you be interested by one ? ), we can have a template specialization for Span that only stores the Span and look at data() to determine if the array is present or not.

The issue with consistency is that depending on the stand point something can be declared consistent or not given that the language is full of inconsistencies.

An array naturally decays to a pointer in pointer/boolean context so from that perspective it would be consistent to have the operator bool() const member function. At the same time, standard/gsl members consider that span should be treated as a regular container and none of the container exposes that operator: they do expose an empty() member function to test if the container is empty or not.

To go back, to the initial issue you raised, in std::span, it is perfectly fine to store nothing in fixed size span. Should the code in this PR change to not accept empty span of non-0 size ?

Agree that the language is utterly full of inconsistencies, and it's alarming that we see stuff like string_view->span being thrashed out in consecutive standards that are (from our point of view) 3-4 standards in the future. string_view being semi-obsolete before we ever get it is just daft.

But whenever they do manage to pin something down, if we're going to copy its name I think we should copy the full semantics, just to not add to the confusion.

I'm also with Herb Sutter on the importance of distinguishing "maybe NULL" and "not-NULL" in the language.

In part that's because I'm aware that if the language could enforce non-NULL for pointers we could get rid of hundreds of runtime checks for NULL that get inserted to cope with unit tests that try passing NULL (sometimes as their only "test"). Unit tests may not believe in argument preconditions, but if we can stop them compiling...

So when I see that the standard has left room for that distinction, I'd like to have the chance to use it. In this form, the PR leaves no nice namespace room for the alternative "non-NULL" semantics. I'd like to reserve the name Span for that, with the alternative being OptionalSpan or equivalent. Then whether this PR actually adds Span or OptionalSpan or both is up to you - whatever you think you currently need?

And OptionalSpan could certainly include bool() as optional<span> does.

kjbracey · 2018-08-20T10:15:58Z

To have a string_view we should have a string first!

Indeed! The client guys do have an internal String they use, but they've been gradually stripping it out in favour of pointer+length to save memory. I've previously suggested that a StringView would solve most of their problems.

It seems your implementation does have helpers like the array<->span comparison that C++ lacks - C++ only has those for string_view, afaict.

pan- · 2018-08-20T11:20:17Z

Standard C++ defines standard comparison operator for spans however it does not define helpers that auto convert arrays into span:

uint8_t foo[] = "hello";

// works
if (mbed::Span<uint8_t>(foo) == foo) { }

// error
if (std::span<uint8_t>(foo) == foo) { }

// works
if (std::span<uint8_t>(foo) == std::span<uint8_t>(foo)) { }

This commit aims to make Span implementation more in line with what is present in N4762: - use appropiate index types where applicable. - use typedefed type inside the class (index_type, reference, pointer, element_type) - assertion where applicable - restrict default construction to Span with extent == 0 or extent == dynamic. - construct span from a range of pointer - remove non const overload of the subscript operator - remove non const overload of the data function - implement subspan function - implement missing first and last function of dynamic span

pan- · 2018-08-21T17:23:49Z

@kjbracey-arm following our offline discussion, I've updated the implementation. to be more in line with the latest standard draft. Apologies if I haven't split it into many commits :(.

kjbracey

This looks a lot better to me. Just editing issues remain, I think.

kjbracey · 2018-08-22T10:13:07Z

platform/Span.h

+     * Extent != 0 .
+     */
+    Span() : _data(NULL) {
+        MBED_STATIC_ASSERT(Extent == 0, "Invalid extent for a Span");


Shame we can't kill the overload suppress. Anyway, this isn't the correct error - should be something like "Cannot default construct a static-extent Span (unless Extent is 0)". Make that less wordy if you can...

Mind you, suppressing the overload would lead to a less-clear error message.

It is possible to kill the overload (see here) but if I can stay away of enable_if and other TMP tricks I do as others may have to touch at the code later.

All in all, static assert kills the overloads if it is selected for instantiation. Given that it is a nullary function and there is only one nullary overload per function name, the only case where the overload is selected is when the default constructor is called. In the end, that doesn't change much to kill it from the overloading set or to kill it if it is instantiated.

Side note: clang implementation use the same construct.

others may have to touch at the code later.

Quite :)

kjbracey · 2018-08-22T10:15:14Z

platform/Span.h

+     */
+    reference operator[](index_type index) const
+    {
+        return _data[index];


Would this be a good place for an MBED_ASSERT range-check on index? Though maybe that should be limited to debug builds, not normal develop.

I thought about it, that is the place where it potentially costs the most. MBED_ASSERT are preserved in develop builds.

Here is some data about the cost of checks: https://godbolt.org/z/E5c1b6
MINIMAL_CHECK tests the upper boundary while FULL_CHECK tests the lower and upper boundary.

Yes, I think it's too much for develop builds. Could do it conditionalised on MBED_DEBUG or a custom flag. Anyway, can be added later.

kjbracey · 2018-08-22T10:17:39Z

platform/Span.h

+     * @param count Number of elements viewed.
+     *
+     * @pre [ptr, ptr + count) must be be a valid range.
+     * @pre count must be equal to extent.


kjbracey · 2018-08-22T10:18:19Z

platform/Span.h

+        _data(elements) { }
+
+    /**
+     * Return the size of the array viewed.


Quite a few references to "array" or "array viewed" in comments where it should now be span.

pan- · 2018-08-24T10:26:18Z

@kjbracey-arm I evaluated a bit more the static assert option and it should be good as long as we have a single conversion constructor. SFINAE would have to be reintroduced if we add conversion functions from other type of containers. af69e1f commit message contains a more detailed explanation.

0xc0170 · 2018-08-24T12:58:04Z

@pan- I can't locate tests and documentation PR for this addition

pan- · 2018-08-24T13:44:03Z

@0xc0170 The number of tests we can set are rather limited and trivial as the type is just a constrained pointer (and optionally a size) to a contiguous sequence. Precondition violation will either generate a failure are runtime or compile time and we cannot test these unfortunately. I can add some test but they will have to be converted to the new unit test framework later.

I will open an entry in the handbook; the class documentation is a good start.

Copy edit file, mostly for consistent capitalization, punctuation and tense.

AnotherButler · 2018-08-24T14:31:28Z

platform/Span.h

+/**
+ * Nonowning view to a sequence of contiguous elements.
+ *
+ * Spans encapsulate a pointer to a sequence of contiguous elements and its size


Query: What does "it" refer to in this sentence?

it refers to the sequence.

Not sure that works - I get it from context, but don't think you can use "it" to reach into the "to" of a clause. It reads as either "encapsulates ((a pointer to a sequence of contiguous elements) and (the pointer's size))" or "encapsulates a pointer to ((a sequence of contiguous elements) and (the sequence's size))".

How about "Spans encapsulate the address and size of a sequence of contiguous elements"? That also avoids the indefiniteness of "a pointer" versus "its length".

Although I still like using the word pointer. How about "Spans encapsulate a pointer and length providing a view over a sequence of contiguous elements".

AnotherButler

I've made some copy edits. Please review them to make sure I didn't accidentally change the meaning of anything.

pan- · 2018-08-24T14:34:21Z

platform/Span.h

 * at runtime.
 */
 #define SPAN_DYNAMIC_EXTENT -1

 /**
- * Non owning view to a sequence of contiguous elements.
+ * Nonowning view to a sequence of contiguous elements.


Shouldn't it be Non-owning ?

Use hyphens when the last letter of the suffix and the first letter of the prefix are the same [proto-oncology, non-negotiable, post-traumatic and so on]. There are some exceptions, such as when the suffix is a proper noun or an acronym. Then, use a hyphen regardless of the first letter of the suffix.

That makes sense. Thanks Amanda.
Looks like a lot of the CS literature spell it wrong.

pan- · 2018-08-24T14:36:05Z

platform/Span.h

 *
 * @par Operations
 *
 * Span objects can be copied and assigned like regular value types with the help
- * of copy constructor and copy assignment (=) operator.
+ * of copy constructor and copy assignment (=) operators.


It is singular in this context; I can rephrase it: "with the help of the copy constructor or the copy assignment (=) operator "

pan- · 2018-08-24T14:40:00Z

platform/Span.h

 *
- * Span can be sliced from the beginning of the sequence (first()), from the end
+ * You can splice Span from the beginning of the sequence (first()), from the end


slice is more commonly used in CS in that context.

That was a typo on my part. My apologies.

pan- · 2018-08-24T14:41:43Z

platform/Span.h

     *
-     * @note OtherElementType(*)[] should be convertible to ElementType(*)[].
+     * @note OtherElementType(*)[] is convertible to ElementType(*)[].


That's the enunciation of a requirement; not the enunciation of the fact. What form should be used ?

Oh, I see. What do you think of "must be"?

That is more precise 👍 .

pan- · 2018-08-24T16:52:59Z

@AnotherButler I've updated the documentation according to our discussion above.

AnotherButler

LGTM

cmonr · 2018-08-25T00:01:27Z

@deepikabhavnani @c1728p9 @SenRamakri @kegilbert Could one of y'all take a look at this and OK it before CI?

cmonr · 2018-08-25T00:02:37Z

Actually, gonna start CI with the hope that it's gets a final OK, since this this blocks #7822

/morph build

mbed-ci · 2018-08-25T01:55:21Z

Build : SUCCESS

Build number : 2901
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7828/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-08-25T04:06:23Z

Exporter Build : SUCCESS

Build number : 2528
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7828/

mbed-ci · 2018-08-25T17:01:04Z

Test : SUCCESS

Build number : 2653
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7828/2653

c1728p9

This looks good to me. Thanks for bringing this main-line @pan-

paul-szczepanek-arm approved these changes Aug 20, 2018

View reviewed changes

0xc0170 added the needs: review label Aug 20, 2018

0xc0170 requested review from kjbracey and a team August 20, 2018 07:45

pan- added 2 commits August 20, 2018 09:13

Span: Cleanup usage of ptrdiff_t and size_t

7f195b8

Span: Improve comparison operator to array by using fixed size Span.

eba1dc6

kjbracey reviewed Aug 20, 2018

View reviewed changes

pan- added 4 commits August 20, 2018 10:29

Span: Make name of dynamic span tag consistent with C++ standard.

43170d2

Span: Fix odd condition in Span::empty.

3fb3173

Span: Fix typo.

9b9d33a

Span: Make type pointer and reference declaration consistent with gui…

6fcf1e8

…deline.

kjbracey reviewed Aug 20, 2018

View reviewed changes

pan- added 2 commits August 20, 2018 11:58

Span: replace reference to Size with Extent.

1f00336

Span: Fix documentation error.

3985fb8

donatieng mentioned this pull request Aug 21, 2018

Add initial NFC support to Mbed OS #7822

Merged

pan- added 2 commits August 22, 2018 09:40

Span: Fix subspan return type

938d802

Span: Improve readability of precondition checks

26b546b

kjbracey reviewed Aug 22, 2018

View reviewed changes

pan- added 2 commits August 22, 2018 13:31

Span: Fix static assert message in default constructor.

f6dd5ea

Span: add boundary check in subscript operator for debug profile

95fc284

kjbracey approved these changes Aug 24, 2018

View reviewed changes

0xc0170 requested a review from AnotherButler August 24, 2018 12:58

Copy edit Span.h

bf08651

Copy edit file, mostly for consistent capitalization, punctuation and tense.

AnotherButler reviewed Aug 24, 2018

View reviewed changes

AnotherButler approved these changes Aug 24, 2018

View reviewed changes

pan- commented Aug 24, 2018

View reviewed changes

Span: Fix documentation.

9d1fd99

pan- mentioned this pull request Aug 24, 2018

Add platform/Span documentation. ARMmbed/mbed-os-5-docs#683

Merged

AnotherButler approved these changes Aug 24, 2018

View reviewed changes

cmonr added needs: CI risk: G and removed needs: review risk: A labels Aug 25, 2018

cmonr added ready for merge and removed needs: CI labels Aug 26, 2018

c1728p9 approved these changes Aug 27, 2018

View reviewed changes

cmonr merged commit c12c69f into ARMmbed:master Aug 27, 2018

0xc0170 removed the ready for merge label Aug 27, 2018

cmonr removed the risk: G label Sep 2, 2018

Platform: Add C++ Span class. #7828

Platform: Add C++ Span class. #7828

Uh oh!

Conversation

pan- commented Aug 19, 2018

Description

Pull request type

Uh oh!

pan- commented Aug 19, 2018

Uh oh!

paul-szczepanek-arm commented Aug 20, 2018

Uh oh!

pan- commented Aug 20, 2018

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan- commented Aug 20, 2018

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan- Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey commented Aug 20, 2018

Uh oh!

pan- commented Aug 20, 2018

Uh oh!

pan- commented Aug 21, 2018

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan- commented Aug 24, 2018

pan- Aug 20, 2018 •

edited

Loading

kjbracey Aug 20, 2018 •

edited

Loading

0xc0170 commented Aug 24, 2018 •

edited

Loading