Support Span<T> conversion #29

gukoff · 2021-05-21T13:56:29Z

Description

Support Span conversion.

REVIEW THE FIRST COMMIT

Motivation and Context

No support existed before.

How Has This Been Tested?

Verified regenerated projects work in Visual Studio.

Screenshots

Types of changes

Improvement (non-breaking change that improves a feature, code or algorithm)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Config and build (change in the configuration and build system, has no impact on code or features)
Dependencies (update dependencies and changes associated, has no impact on code or features)
Unit Tests (work on Unit Tests, has no impact on code or features)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

gukoff · 2021-05-21T13:58:00Z

src/nanoFramework.IoT.Device.CodeConverter/Program.cs

@@ -682,21 +691,24 @@ string outputDirectory
                    string line;
                    while (null != (line = input.ReadLine()))
                    {
-                        foreach (var replacement in replacements)
+                        // Replacing longer matches first is a safeguard heuristic.


Everything below this line is just refactoring that creeped into this PR

Ellerbach

Please see my comments.
And please check the template here: https://github.com/nanoframework/CoreLibrary/blob/develop/nanoFramework.CoreLibrary/System/SpanByte.cs

Ellerbach · 2021-05-21T14:24:50Z

src/nanoFramework.IoT.Device.CodeConverter/GenericsTemplates/Span.cs

+        {
+            if (array != null)
+            {
+                if ((start + length > array.Length) || (start >= array.Length))


should be this: if ((length > array.Length - start) || (start > array.Length))

Check things here: https://github.com/nanoframework/CoreLibrary/blob/develop/nanoFramework.CoreLibrary/System/SpanByte.cs

Can start be equal to array length, unless length=0?

This looks correct:

if (start < 0 || length < 0 || start + length > array.Length || (start == array.Length && array.Length > 0))

Ellerbach · 2021-05-21T14:26:34Z

src/nanoFramework.IoT.Device.CodeConverter/GenericsTemplates/Span.cs

+        {
+            get
+            {
+                int realIndex = _start + index;


the get should be:

if (index > _length) { throw new ArgumentOutOfRangeException(); } } return _array[_start + index];

Ah, I see. Length is not the length of the array.

By the way it's >=, not >

Good catch for this one. Mind a PR on mscorlib? :-)

Ellerbach · 2021-05-21T14:27:21Z

src/nanoFramework.IoT.Device.CodeConverter/GenericsTemplates/Span.cs

+
+            set
+            {
+                int realIndex = _start + index;


set should be:

if (index > _length) { throw new ArgumentOutOfRangeException(); } _array[_start + index] = value;

Ellerbach · 2021-05-21T14:28:27Z

src/nanoFramework.IoT.Device.CodeConverter/GenericsTemplates/Span.cs

+        /// <exception cref="System.ArgumentOutOfRangeException">start is less than zero or greater than System.Span.Length.</exception>
+        public Span<T> Slice(int start)
+        {
+            return Slice(start, _length - start);


missing the check for the length

if ((start > _length) || (start < 0)) { throw new ArgumentOutOfRangeException(); } return new SpanByte(_array, _start + start, _length - start);

It should check it in the "main" metod Slice(start, length)

Ellerbach · 2021-05-21T14:29:31Z

src/nanoFramework.IoT.Device.CodeConverter/GenericsTemplates/Span.cs

+        /// Defines an implicit conversion of a System.Span to a System.ReadOnlySpan.
+        /// </summary>
+        /// <param name="span">The object to convert to a System.ReadOnlySpan.</param>
+        public static implicit operator ReadOnlySpan<T>(Span<T> span)


remove this one, we don't need ReadOnlySpan, we've simplify and get read of the ReadOnlySpan to only have Span

gukoff · 2021-05-21T16:03:31Z

Made edits in Span.cs
ignored SpanByte
added XML-docs explaining the logic

gukoff · 2021-05-21T16:13:11Z

If I see it right, SpanByte in mscorlib currently has a bug and allows access 1 cell beyond the available length.

If the version in this PR looks good, I can move it to core library.

Ellerbach · 2021-05-21T16:28:16Z

src/nanoFramework.IoT.Device.CodeConverter/GenericsTemplates/Span.cs

+        /// <exception cref="System.ArgumentOutOfRangeException">start is less than zero or greater than System.Span.Length.</exception>
+        public Span<T> Slice(int start)
+        {
+            return Slice(start, _length - start);


Ellerbach · 2021-05-21T16:29:35Z

@gukoff thanks! do you mind resolving the conflicts so we can merge this one soon?

nfbot added the Type: enhancement New feature or request label May 21, 2021

gukoff commented May 21, 2021

View reviewed changes

Ellerbach reviewed May 21, 2021

View reviewed changes

gukoff force-pushed the span branch from eb48cf4 to 00e9f8e Compare May 21, 2021 15:52

gukoff requested a review from Ellerbach May 21, 2021 16:02

Ellerbach approved these changes May 21, 2021

View reviewed changes

Ellerbach enabled auto-merge (squash) May 21, 2021 16:28

Konstantin Gukov added 2 commits May 21, 2021 19:53

Support Span<T> same as List<T>

42d5599

Regenerate devices

76d7eaa

auto-merge was automatically disabled May 21, 2021 17:54
Head branch was pushed to by a user without write access

gukoff force-pushed the span branch from 00e9f8e to 76d7eaa Compare May 21, 2021 17:54

Ellerbach enabled auto-merge (squash) May 21, 2021 17:55

Ellerbach merged commit ad489d5 into nanoframework:develop May 21, 2021

gukoff mentioned this pull request May 21, 2021

Fix access beyond array boundaries in SpanByte nanoframework/CoreLibrary#140

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Span<T> conversion #29

Support Span<T> conversion #29

gukoff commented May 21, 2021 •

edited

Loading

gukoff May 21, 2021

Ellerbach left a comment

Ellerbach May 21, 2021

gukoff May 21, 2021 •

edited

Loading

gukoff May 21, 2021 •

edited

Loading

Ellerbach May 21, 2021

gukoff May 21, 2021 •

edited

Loading

gukoff May 21, 2021

Ellerbach May 21, 2021 •

edited

Loading

Ellerbach May 21, 2021

Ellerbach May 21, 2021

gukoff May 21, 2021

Ellerbach May 21, 2021

Ellerbach May 21, 2021

gukoff commented May 21, 2021

gukoff commented May 21, 2021

Ellerbach May 21, 2021

Ellerbach commented May 21, 2021

Support Span<T> conversion #29

Support Span<T> conversion #29

Conversation

gukoff commented May 21, 2021 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots

Types of changes

Checklist:

Choose a reason for hiding this comment

Ellerbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gukoff May 21, 2021 • edited Loading

Choose a reason for hiding this comment

gukoff May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gukoff May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ellerbach May 21, 2021 • edited Loading

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

gukoff commented May 21, 2021

gukoff commented May 21, 2021

Choose a reason for hiding this comment

Ellerbach commented May 21, 2021

gukoff commented May 21, 2021 •

edited

Loading

gukoff May 21, 2021 •

edited

Loading

gukoff May 21, 2021 •

edited

Loading

gukoff May 21, 2021 •

edited

Loading

Ellerbach May 21, 2021 •

edited

Loading