New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Platform: Add C++ Span class. #7828

Merged
merged 23 commits into from Aug 27, 2018

Conversation

Projects
None yet
8 participants
@pan-
Member

pan- commented Aug 19, 2018

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
Platform: Add C++ Span class.
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-

This comment has been minimized.

Member

pan- commented Aug 19, 2018

@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

This comment has been minimized.

Member

paul-szczepanek-arm commented Aug 20, 2018

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

@pan-

This comment has been minimized.

Member

pan- commented Aug 20, 2018

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;
}

@0xc0170 0xc0170 requested review from kjbracey-arm and ARMmbed/mbed-os-core Aug 20, 2018

@kjbracey-arm

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.

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

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 20, 2018

Contributor

Search-and-replace fail on the name.

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

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 20, 2018

Contributor

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

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

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 20, 2018

Contributor

Formatting rules say pushing the *& to the right.

* @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.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 20, 2018

Contributor

(@p)? array_ptr

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

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 20, 2018

Contributor

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) ...)

This comment has been minimized.

@pan-

pan- Aug 20, 2018

Member

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.

@pan-

This comment has been minimized.

Member

pan- commented Aug 20, 2018

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-arm

Few more review notes.

* 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>

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 20, 2018

Contributor

In C++ the template parameter is also Extent.

* @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

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 20, 2018

Contributor

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?

This comment has been minimized.

@pan-

pan- Aug 20, 2018

Member

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

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

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 20, 2018

Contributor

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.

This comment has been minimized.

@pan-

pan- Aug 20, 2018

Member

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.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 20, 2018

Contributor

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.

This comment has been minimized.

@pan-

pan- Aug 20, 2018

Member

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 ?

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 21, 2018

Contributor

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-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Aug 20, 2018

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-

This comment has been minimized.

Member

pan- commented Aug 20, 2018

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)) { }
Span: Improve consistency with standard.
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-

This comment has been minimized.

Member

pan- commented Aug 21, 2018

@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-arm

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

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

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 22, 2018

Contributor

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.

This comment has been minimized.

@pan-

pan- Aug 22, 2018

Member

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.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 22, 2018

Contributor

others may have to touch at the code later.

Quite :)

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

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 22, 2018

Contributor

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.

This comment has been minimized.

@pan-

pan- Aug 22, 2018

Member

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.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 22, 2018

Contributor

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.

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

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 22, 2018

Contributor

Extent

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

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 22, 2018

Contributor

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 24, 2018

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

@0xc0170 0xc0170 requested a review from AnotherButler Aug 24, 2018

@pan-

This comment has been minimized.

Member

pan- commented Aug 24, 2018

@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 Span.h
Copy edit file, mostly for consistent capitalization, punctuation and tense.
/**
* Nonowning view to a sequence of contiguous elements.
*
* Spans encapsulate a pointer to a sequence of contiguous elements and its size

This comment has been minimized.

@AnotherButler

AnotherButler Aug 24, 2018

Contributor

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

This comment has been minimized.

@pan-

pan- Aug 24, 2018

Member

it refers to the sequence.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 27, 2018

Contributor

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.

* at runtime.
*/
#define SPAN_DYNAMIC_EXTENT -1
/**
* Non owning view to a sequence of contiguous elements.
* Nonowning view to a sequence of contiguous elements.

This comment has been minimized.

@pan-

pan- Aug 24, 2018

Member

Shouldn't it be Non-owning ?

This comment has been minimized.

@AnotherButler

AnotherButler Aug 24, 2018

Contributor

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.

This comment has been minimized.

@pan-

pan- Aug 24, 2018

Member

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

*
* @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.

This comment has been minimized.

@pan-

pan- Aug 24, 2018

Member

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

*
* 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

This comment has been minimized.

@pan-

pan- Aug 24, 2018

Member

slice is more commonly used in CS in that context.

This comment has been minimized.

@AnotherButler

AnotherButler Aug 24, 2018

Contributor

That was a typo on my part. My apologies.

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

This comment has been minimized.

@pan-

pan- Aug 24, 2018

Member

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

This comment has been minimized.

@AnotherButler

AnotherButler Aug 24, 2018

Contributor

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

This comment has been minimized.

@pan-

pan- Aug 24, 2018

Member

That is more precise 👍 .

@pan-

This comment has been minimized.

Member

pan- commented Aug 24, 2018

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

@AnotherButler

LGTM

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 25, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 25, 2018

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

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 25, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

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

@c1728p9

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

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

15 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0.0%) RAM(+0.0%)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job was successful
Details
travis-ci/astyle Passed, 588 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9081 cycles (-123 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the risk: G label Sep 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment