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

Add Encoding.GetBytes(string, offset, count) #8651

Merged
merged 16 commits into from
Dec 19, 2016
Merged

Add Encoding.GetBytes(string, offset, count) #8651

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
f51fcef
Add Encoding.GetBytes(string, offset, count)
AlexRadch Dec 15, 2016
ee140f0
Optimize GetBytes
AlexRadch Dec 15, 2016
8365eca
// Deligate parameters validation to string.ToCharArray method
AlexRadch Dec 15, 2016
c307bea
Fix model.xml
AlexRadch Dec 15, 2016
2fe7879
Fix
AlexRadch Dec 15, 2016
01a1a49
Made unsafe
AlexRadch Dec 15, 2016
29aaf03
Environment.GetResourceString("ArgumentNull_String")
AlexRadch Dec 15, 2016
4dce9b8
Debug.Assert(byteCount >= bytesReceived);
AlexRadch Dec 15, 2016
1796c4e
// encoding == Encoding.UTF8
AlexRadch Dec 15, 2016
b2aeeac
fixed (char *pChar = string)
AlexRadch Dec 15, 2016
fab56e7
fixed (char* pChar = str)
AlexRadch Dec 15, 2016
3ff89ba
merge
AlexRadch Dec 15, 2016
6adee8a
if (byteCount == 0) { return Array.Empty<byte>(); }
AlexRadch Dec 16, 2016
f0c8bf8
fixed (byte* pBytes = bytes)
AlexRadch Dec 16, 2016
af21b10
non virtual
AlexRadch Dec 17, 2016
523554c
fixed (byte* pBytes = &bytes[0])
AlexRadch Dec 19, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/mscorlib/model.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7763,10 +7763,12 @@
<Member Name="GetByteCount(System.Char[])" />
<Member Name="GetByteCount(System.Char[],System.Int32,System.Int32)" />
<Member Name="GetByteCount(System.String)" />
<Member Name="GetByteCount(System.String,System.Int32,System.Int32)" />
<Member Name="GetBytes(System.Char[])" />
<Member Name="GetBytes(System.Char[],System.Int32,System.Int32)" />
<Member Name="GetBytes(System.Char[],System.Int32,System.Int32,System.Byte[],System.Int32)" />
<Member Name="GetBytes(System.String)" />
<Member Name="GetBytes(System.String,System.Int32,System.Int32)" />
<Member Name="GetBytes(System.String,System.Int32,System.Int32,System.Byte[],System.Int32)" />
<Member Name="GetBytes(System.Char*,System.Int32,System.Byte*,System.Int32)" />
<Member Name="GetCharCount(System.Byte[])" />
Expand Down
71 changes: 69 additions & 2 deletions src/mscorlib/src/System/Text/Encoding.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -869,7 +869,7 @@ public virtual int GetByteCount(char[] chars)
[Pure]
public virtual int GetByteCount(String s)
{
if (s==null)
if (s == null)
throw new ArgumentNullException(nameof(s));
Contract.EndContractBlock();

Expand All @@ -884,6 +884,34 @@ public virtual int GetByteCount(String s)
[Pure]
public abstract int GetByteCount(char[] chars, int index, int count);

// Returns the number of bytes required to encode a string range.
//
[Pure]
public int GetByteCount(string s, int index, int count)
{
if (s == null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should validate all passed parameters to this method too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are validated in next line char[] chars = s.ToCharArray(index, count);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

although I prefer validating the parameters in this method so the thrown exception can be more clear about the problem but I see the perf benefit too. so I am ok to keep what you have but please add assert in the methods you are introducing

In reply to: 92667264 [](ancestors = 92667264)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The argument validation will be needed here once the implementation changed to be the efficient one without the allocation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added comment // Deligate parameters validation to string.ToCharArray method

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexRadch I assume you are going to change the implementation to use the unsafe code instead of the allocation as @jkotas mentioned. right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will

throw new ArgumentNullException(nameof(s),
Environment.GetResourceString("ArgumentNull_String"));
if (index < 0)
throw new ArgumentOutOfRangeException(nameof(index),
Environment.GetResourceString("ArgumentOutOfRange_NeedNonNegNum"));
if (count < 0)
throw new ArgumentOutOfRangeException(nameof(count),
Environment.GetResourceString("ArgumentOutOfRange_NeedNonNegNum"));
if (index > s.Length - count)
throw new ArgumentOutOfRangeException(nameof(index),
Environment.GetResourceString("ArgumentOutOfRange_IndexCount"));
Contract.EndContractBlock();

unsafe
{
fixed (char* pChar = s)
{
return GetByteCount(pChar + index, count);
}
}
}

// We expect this to be the workhorse for NLS encodings
// unfortunately for existing overrides, it has to call the [] version,
// which is really slow, so this method should be avoided if you're calling
Expand Down Expand Up @@ -978,10 +1006,49 @@ public virtual byte[] GetBytes(String s)
return bytes;
}

// Returns a byte array containing the encoded representation of the given
// string range.
//
[Pure]
public byte[] GetBytes(string s, int index, int count)
{
if (s == null)
throw new ArgumentNullException(nameof(s),
Environment.GetResourceString("ArgumentNull_String"));
if (index < 0)
throw new ArgumentOutOfRangeException(nameof(index),
Environment.GetResourceString("ArgumentOutOfRange_NeedNonNegNum"));
if (count < 0)
throw new ArgumentOutOfRangeException(nameof(count),
Environment.GetResourceString("ArgumentOutOfRange_NeedNonNegNum"));
if (index > s.Length - count)
throw new ArgumentOutOfRangeException(nameof(index),
Environment.GetResourceString("ArgumentOutOfRange_IndexCount"));
Contract.EndContractBlock();

unsafe
{
fixed (char* pChar = s)
{
int byteCount = GetByteCount(pChar + index, count);
if (byteCount == 0)
return Array.Empty<byte>();

byte[] bytes = new byte[byteCount];
fixed (byte* pBytes = &bytes[0])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried

            fixed (byte *pBytes = bytes)
            {
            }

and it is working. could you fix this one? I think this is the last change requested in this PR

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works but it generates worse code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I missed the previous comments regarding this part. I am ok with the change then.

@jkotas are you ok now with the changes so I can merge it?

{
int bytesReceived = GetBytes(pChar + index, count, pBytes, byteCount);
Debug.Assert(byteCount == bytesReceived);
}
return bytes;
}
}
}

public virtual int GetBytes(String s, int charIndex, int charCount,
byte[] bytes, int byteIndex)
{
if (s==null)
if (s == null)
throw new ArgumentNullException(nameof(s));
Contract.EndContractBlock();
return GetBytes(s.ToCharArray(), charIndex, charCount, bytes, byteIndex);
Expand Down