Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Change the semantics of Span ToString to return the contents for T=char - #26726

Merged
jkotas merged 5 commits into
dotnet:masterfrom
ahsonkhan:UpdateToString
Feb 5, 2018
Merged

Change the semantics of Span ToString to return the contents for T=char#26726
jkotas merged 5 commits into
dotnet:masterfrom
ahsonkhan:UpdateToString

Conversation

@ahsonkhan

@ahsonkhan ahsonkhan commented Feb 1, 2018

Copy link
Copy Markdown

Fixes https://github.com/dotnet/corefx/issues/26584

Related PR: dotnet/coreclr#16143

This PR supersedes #26663

It is blocked:
After dotnet/coreclr#16087 is merged and CoreFX is updated to use the new coreclr version, this PR can be merged.

cc @pakrym, @jkotas, @stephentoub, @dotnet/corefxlab-contrib

@ahsonkhan

ahsonkhan commented Feb 1, 2018

Copy link
Copy Markdown
Author

I don't understand why I am seeing these compiler errors for netfx when I do:

Assert.Equal("System.Span<int>[3]", span.ToString());
Span\ToString.cs(17,49): error CS0029: Cannot implicitly convert type 'System.Span<int>' to 'System.ValueType'

Where is the implicit conversion happening?

And this error only occurs for Span (not ReadOnlySpan).

@jkotas

jkotas commented Feb 1, 2018

Copy link
Copy Markdown
Member

Where is the implicit conversion happening?

There is implicit conversion because of System.Memory reference assembly is missing the ToString overload.

@ahsonkhan

Copy link
Copy Markdown
Author

There is implicit conversion because of System.Memory reference assembly is missing the ToString overload.

Yes! For both Span and Memory.

Also, ReadOnlyMemory in S.Runtime ref is also missing it: https://github.com/dotnet/corefx/blob/master/src/System.Runtime/ref/System.Runtime.cs#L1933

@pakrym

pakrym commented Feb 1, 2018

Copy link
Copy Markdown

Visual Studio treats strings and ToString differently when returning non printable characters - it escapes them in strings but not ToString results, for example:

image

Does the same thing happen to spans in your change?

@khellang

khellang commented Feb 2, 2018

Copy link
Copy Markdown
Member

You can force the debugger to show the raw output, using [DebuggerDisplay]:

[DebuggerDisplay("{ToString(),raw}")]
public class c
{
    public override string ToString()
    {
        return "\r\b\0";
    }
}

Ouput:

image

@ahsonkhan

ahsonkhan commented Feb 3, 2018

Copy link
Copy Markdown
Author

You can force the debugger to show the raw output

I will make this change outside this PR to avoid mirroring issues (along with removing the redundant DebuggerDisplay property -

private string DebuggerDisplay => string.Format("System.Span<{0}>[{1}]", typeof(T).Name, _length);
).

@ahsonkhan

Copy link
Copy Markdown
Author

@dotnet-bot test this please

@ahsonkhan

Copy link
Copy Markdown
Author

@dotnet-bot test UWP CoreCLR x64 Debug Build

@jkotas

jkotas commented Feb 5, 2018

Copy link
Copy Markdown
Member

UWP failure is https://github.com/dotnet/corefx/issues/26802 (CI have not picked up the new pipeline definition for existing PRs)

@jkotas
jkotas merged commit 928d525 into dotnet:master Feb 5, 2018
@ahsonkhan
ahsonkhan deleted the UpdateToString branch February 5, 2018 04:02
@karelz karelz added this to the 2.1.0 milestone Feb 5, 2018
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…ar (dotnet/corefx#26726)

* Change the semantics of Span ToString to return the contents for T=char

* Updating ToString tests.

* Fix the expected value in tests and add Span<string> test.

* Add missing ToString methods to the reference assemblies.


Commit migrated from dotnet/corefx@928d525
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants