Add functions to create a Span from an std::array

[marcemmers]

Summary of changes

Add functions to create a Span from an std::array to further align with the std::span implementation.

Impact of changes

Migration actions required

Documentation

None

---

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[X] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

---

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

---

Reviewers

---

[ciarmcom]

@marcemmers, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

[kjbracey]

If you're in the mood for spans, I'd love to see this migrated to mstd::span in the cxxsupport to follow the newer Mbed model of tracking modern C++ - would also give you the opportunity to redo the ptrdiff_t/size_t thing that changed during C++20 drafting without upsetting existing mbed::Span users.

[kjbracey]

This has been open for ages, not sure what's holding it up: #13085

I'd suggest using static_assert directly in this new code.

[kjbracey]

You can now rely on the std type traits stuff, no need for the span_detail any more.

[marcemmers]

If you're in the mood for spans, I'd love to see this migrated to mstd::span in the cxxsupport to follow the newer Mbed model of tracking modern C++ - would also give you the opportunity to redo the ptrdiff_t/size_t thing that changed during C++20 drafting without upsetting existing mbed::Span users.

I can see what I can do but have a couple of questions then though.

• Is it an option to use enable_if in this code? Because that isn't used up until now.
• I suppose you mean to leave the mbed::Span as is and add an mstd::span implementation?

[pan-]

@marcemmers How do you want to use enable_if ?

[kjbracey]

mbed::Span predates C++11 support in Mbed OS, so has local implementations of stuff that are no longer required. We now can rely on a C++14 compiler and most of the library, including std::enable_if_t. (Backup support is available in cxxsupport). You can assume full C++14 type_traits now.

Yes, I'm suggesting you could make an updated version in cxxsupport, and leave this for backward compatibility (and maybe deprecate).

[marcemmers]

@marcemmers How do you want to use enable_if ?

Instead of adding a static_assert to certain constructors and functions to check if they are valid I would prevent the usage by enable_if.

[pan-]

I remember I used static_assert in Span(const Span<OtherElementType, Extent> &other) to improve the user experience and give a better error message to the user. That's something to keep in mind when adding enable_if, if none of the constructor are selected, errors can become cryptic very quickly if there is a lot of overloads.

[kjbracey]

Yes, that makes sense when it hits the right wrong overload. You can be told off with a sensible message.

But the catch is that sometimes there's a different overload that would work, so the enable_if stops it picking this invalid one, letting it unambiguously land on another one.

I'm absolutely rubbish at analysing any of this stuff, so when doing C++-standard-imitation things I just stick to what the standard says, which is almost exclusively the enable_if form ("this participates in overload resolution only if ...."). I can't really visualise when having extra overload candidates might break valid code, and I haven't tried to analyse this API.

[marcemmers]

I have a basic implementation, most of the stuff should be there: https://github.com/marcemmers/mbed-os/blob/add-mstd-span/platform/cxxsupport/mstd_span

I don't really know how to implement the container constructor though.

Also, I didn't implement some of the constructors correctly because they need some constraints I couldn't really figure out so for now I used the span's element_type pointer for that.

So what's the best way forward now? I suppose there will be plenty to do before it is ready to be merged, so can you review it on my repo or should I just make a PR and work from there?

I remember I used static_assert in Span(const Span<OtherElementType, Extent> &other) to improve the user experience and give a better error message to the user. That's something to keep in mind when adding enable_if, if none of the constructor are selected, errors can become cryptic very quickly if there is a lot of overloads.

I can see what you mean. I'll have to look into that then. Guess its also dependant on the compiler used what kind of errors you get.

[kjbracey]

Note that the C++20 spec reworked the Standardese for methods to quickly address these common forms - "Constraints" wants an enable_if, and "Mandates" wants a static_assert.

[marcemmers]

So I implemented the container constructor and changed some constructors to take a template argument as type instead of the span::pointer type.

This should be more like the standard already.

There is a warning when I try to get a subspan with offset 0 because there is a pointless comparison there:
MBED_ASSERT(Offset <= size()). There is a way to fix it using templates but its a couple of lines to fix a simple warning (see https://stackoverflow.com/questions/38255313/pointless-comparison-warning-in-template-with-certain-values). I prefer to fix it but don't know what your views are on the matter?

[kjbracey]

I'd suggest making a separate PR for the mstd::span.

There is a warning when I try to get a subspan with offset 0

I'd say using subspan with constant offset zero is a bit weird anyway - why not use first?.

What if you change it to MBED_ASSERT(Offset == 0 || Offset <= size())? That short-circuits the size check, and maybe the warning if you're lucky.

[marcemmers]

Opened a PR: #13881 for the mstd::span

I suppose this one can be closed then?

[kjbracey]

I suppose this one can be closed then?

Don't know - this one is a possible extension for people who aren't interested in reworking their code, or it could be used for a Mbed 5.15 backport?

[0xc0170]

I'll close this. It can be reopened if anyone objects this still should get in with #13881