Add std::span implementation for cxxsupport #13881

marcemmers · 2020-11-09T12:08:33Z

Summary of changes

Adds std::span implementation to cxxsupport folder in namespace mstd.
See discussion in #13868.

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

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

Reviewers

@kjbracey-arm

ciarmcom · 2020-11-09T12:30:25Z

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

kjbracey

This looks pretty good to me. A few comments for starters.

platform/cxxsupport/mstd_span

kjbracey · 2020-11-09T12:32:43Z

platform/cxxsupport/mstd_span

+    typename mstd::enable_if_t<detail::is_container<R>::value &&
+            detail::is_compatible<R&, ElementType>::value, int> = 0>
+    constexpr span(R&& r) :
+        _storage( mstd::data( r ), mstd::size( r ))


No spaces inside parentheses.

Still assorted spaces here, and line 206.

pan-

Thank you @marcemmers for this submission; it looks quite good, I really like the use of the storage class.
I have few questions:

Should we mark mbed::Span as deprecated ? I'm not seeing much value in promoting both.
What is the transition path from mbed::Span to mstd::span ?
@kjbracey-arm What are the expectations in term of documentation for mstd ?

I'll review the details of the code tomorrow.

platform/cxxsupport/mstd_span

kjbracey · 2020-11-10T05:50:29Z

Should we mark mbed::Span as deprecated ? I'm not seeing much value in promoting both.

Yes, that should follow on.

What is the transition path from mbed::Span to mstd::span ?

Don't quite understand the question. Before or when we deprecate it, we'd need to convert the Mbed code base.

@kjbracey-arm What are the expectations in term of documentation for mstd ?

cxxsupport could do with a proper docs site section - at the minute all it has is its own README.md and comments in the files themselves.

However, for the individual components like this, I believe you don't really need to say much or anything usually. Everything in mstd is supposed to function exactly as its std equivalent, following the principles described in the README.md. If that's what a class does, no special documentation necessary, just as a toolchain wouldn't include any for <span>. Some parts that are replacements for C++14 bits with different characteristics, like mstd::atomic, need comments explaining those and why you wouldn't just use std, but straight backports like this don't need anything, IMO.

Extensions like make_span should get some Doxygen text. Although I'm not sure these files make it to any generated Doxygen output...

platform/cxxsupport/mstd_span

pan- · 2020-11-11T11:48:23Z

Before or when we deprecate it, we'd need to convert the Mbed code base.

I was thinking of users of Mbed OS that use mbed::Span today. We should discover more when mbed::Span is replaced with mstd::span.

0xc0170 · 2020-11-12T09:45:35Z

As this is new feature, if we want to have this in the next release (it will be feature release), please let us know (code freeze end of today).

marcemmers · 2020-11-23T07:34:41Z

So what's the plan here guys? Should I add the doxygen for the make_Span functions. Is there anything else I can do?

0xc0170 · 2020-11-30T16:26:45Z

@marcemmers doxygen docs always good to have.

@pan- @kjbracey-arm What would you suggest, what else is to be done here ?

pan- · 2020-11-30T16:43:39Z

@0xc0170 It seems fairly complete to me.

kjbracey

I'm pretty happy with this - just want to lose the nonconformant as_bytes.

platform/cxxsupport/mstd_span

adbridge · 2021-01-07T15:38:09Z

@marcemmers can you take a look at the review comments please.

marcemmers · 2021-01-14T07:51:23Z

@marcemmers can you take a look at the review comments please.

Thanks for the reminder, I'll take a look this weekend.

Pull request has been modified.

0xc0170 · 2021-01-18T10:08:03Z

Thank you @marcemmers , I started CI

mbed-ci · 2021-01-18T12:04:31Z

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	❌
jenkins-ci/mbed-os-ci_build-example-ARM	❌
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	❌
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	❌
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	❌

kjbracey

Nearly there - shouldn't take much longer if we can just get the iteration cycle down :)

Main problem is the missing definitions - see comment thread elsewhere.

platform/cxxsupport/mstd_iterator

platform/cxxsupport/mstd_span

kjbracey · 2021-01-18T12:35:54Z

platform/cxxsupport/mstd_span

+    typename mstd::enable_if_t<detail::is_container<R>::value &&
+            detail::is_compatible<R&, ElementType>::value, int> = 0>
+    constexpr span(R&& r) :
+        _storage( mstd::data( r ), mstd::size( r ))


Still assorted spaces here, and line 206.

Keep make_span functions even when <span> is available for compatibility

Pull request has been modified.

0xc0170 · 2021-01-18T16:35:39Z

CI started

mbed-ci · 2021-01-18T19:42:30Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-test	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

marcemmers mentioned this pull request Nov 9, 2020

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

Closed

ciarmcom added the release-type: feature label Nov 9, 2020

ciarmcom requested review from kjbracey, pan- and a team November 9, 2020 12:30

ciarmcom added the needs: review label Nov 9, 2020

kjbracey reviewed Nov 9, 2020

View reviewed changes

pan- reviewed Nov 9, 2020

View reviewed changes

platform/cxxsupport/mstd_span Show resolved Hide resolved

ladislas reviewed Nov 9, 2020

View reviewed changes

platform/cxxsupport/mstd_span Outdated Show resolved Hide resolved

kjbracey reviewed Nov 10, 2020

View reviewed changes

platform/cxxsupport/mstd_span Show resolved Hide resolved

kjbracey previously requested changes Dec 1, 2020

View reviewed changes

mergify bot added needs: work and removed needs: review labels Dec 1, 2020

Marc Emmers added 7 commits January 16, 2021 20:02

Adds basic span implementation

2f0bfd6

Fix endif comment

c38df18

Add iter_reference_t to iterator header

11573a9

Correct span constructors

e93ebc4

Add container constructor and make_span

4097b9d

Prevent warning on subspan<0,N>

c89c458

Fix namespaces and span prototype

1931f4e

marcemmers force-pushed the add-mstd-span branch from 68bd6a9 to 78f2cfc Compare January 16, 2021 20:04

0xc0170 added needs: CI and removed needs: work labels Jan 18, 2021

0xc0170 previously approved these changes Jan 18, 2021

View reviewed changes

mergify bot added needs: work and removed needs: CI labels Jan 18, 2021

kjbracey previously requested changes Jan 18, 2021

View reviewed changes

Marc Emmers added 4 commits January 18, 2021 14:33

Add to list of backports

8f94feb

Fix missing defintion

d5459ea

Backport dynamic extent when including <span>

a826c89

Keep make_span functions even when <span> is available for compatibility

Fix last extra space issues

8551e93

Fix make_span text in comment block

a8ea775

kjbracey approved these changes Jan 18, 2021

View reviewed changes

mergify bot added needs: CI and removed needs: work labels Jan 18, 2021

mergify bot added ready for merge and removed needs: CI labels Jan 18, 2021

0xc0170 merged commit 860267c into ARMmbed:master Jan 19, 2021

mergify bot removed the ready for merge label Jan 19, 2021

mbedmain added release-version: 6.7.0 Release-pending and removed release-type: feature Release-pending labels Jan 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add std::span implementation for cxxsupport #13881

Add std::span implementation for cxxsupport #13881

marcemmers commented Nov 9, 2020 •

edited by 0xc0170

Loading

ciarmcom commented Nov 9, 2020

kjbracey left a comment

kjbracey Nov 9, 2020

kjbracey Jan 18, 2021

pan- left a comment

kjbracey commented Nov 10, 2020

pan- commented Nov 11, 2020

0xc0170 commented Nov 12, 2020

marcemmers commented Nov 23, 2020

0xc0170 commented Nov 30, 2020

pan- commented Nov 30, 2020

kjbracey left a comment

adbridge commented Jan 7, 2021

marcemmers commented Jan 14, 2021

0xc0170 commented Jan 18, 2021

mbed-ci commented Jan 18, 2021

kjbracey left a comment •

edited

Loading

kjbracey Jan 18, 2021

0xc0170 commented Jan 18, 2021

mbed-ci commented Jan 18, 2021

Add std::span implementation for cxxsupport #13881

Add std::span implementation for cxxsupport #13881

Conversation

marcemmers commented Nov 9, 2020 • edited by 0xc0170 Loading

Summary of changes

Impact of changes

Migration actions required

Documentation

None

Pull request type

Test results

Reviewers

@kjbracey-arm

ciarmcom commented Nov 9, 2020

kjbracey left a comment

Choose a reason for hiding this comment

kjbracey Nov 9, 2020

Choose a reason for hiding this comment

kjbracey Jan 18, 2021

Choose a reason for hiding this comment

pan- left a comment

Choose a reason for hiding this comment

kjbracey commented Nov 10, 2020

pan- commented Nov 11, 2020

0xc0170 commented Nov 12, 2020

marcemmers commented Nov 23, 2020

0xc0170 commented Nov 30, 2020

pan- commented Nov 30, 2020

kjbracey left a comment

Choose a reason for hiding this comment

adbridge commented Jan 7, 2021

marcemmers commented Jan 14, 2021

0xc0170 commented Jan 18, 2021

mbed-ci commented Jan 18, 2021

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

kjbracey left a comment • edited Loading

Choose a reason for hiding this comment

kjbracey Jan 18, 2021

Choose a reason for hiding this comment

0xc0170 commented Jan 18, 2021

mbed-ci commented Jan 18, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

marcemmers commented Nov 9, 2020 •

edited by 0xc0170

Loading

kjbracey left a comment •

edited

Loading