Skip to content

Made char.IsAscii public #41396

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
GrabYourPitchforks merged 2 commits into from
Aug 31, 2020
Merged

Made char.IsAscii public #41396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cbeba0c
Made char.IsAscii public
gfoidl Aug 26, 2020
bf68f4e
Updated ref-assembly
gfoidl Aug 26, 2020
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
10 changes: 5 additions & 5 deletions src/libraries/System.Private.CoreLib/src/System/Char.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,16 @@ namespace System
};

// Return true for all characters below or equal U+00ff, which is ASCII + Latin-1 Supplement.
private static bool IsLatin1(char ch) => (uint)ch < (uint)Latin1CharInfo.Length;
private static bool IsLatin1(char c) => (uint)c < (uint)Latin1CharInfo.Length;

// Return true for all characters below or equal U+007f, which is ASCII.
private static bool IsAscii(char ch) => (uint)ch <= '\x007f';
public static bool IsAscii(char c) => (uint)c <= '\x007f';

// Return the Unicode category for Unicode character <= 0x00ff.
private static UnicodeCategory GetLatin1UnicodeCategory(char ch)
private static UnicodeCategory GetLatin1UnicodeCategory(char c)
{
Debug.Assert(IsLatin1(ch), "char.GetLatin1UnicodeCategory(): ch should be <= 00ff");
return (UnicodeCategory)(Latin1CharInfo[ch] & UnicodeCategoryMask);
Debug.Assert(IsLatin1(c), "char.GetLatin1UnicodeCategory(): c should be <= 00ff");
return (UnicodeCategory)(Latin1CharInfo[c] & UnicodeCategoryMask);
}

//
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Runtime/ref/System.Runtime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -728,6 +728,7 @@ public CannotUnloadAppDomainException(string? message, System.Exception? innerEx
private readonly char _dummyPrimitive;
public const char MaxValue = '\uFFFF';
public const char MinValue = '\0';
public static bool IsAscii(System.Char c) { throw null; }
public int CompareTo(System.Char value) { throw null; }
public int CompareTo(object? value) { throw null; }
public static string ConvertFromUtf32(int utf32) { throw null; }
Expand Down
9 changes: 9 additions & 0 deletions src/libraries/System.Runtime/tests/System/CharTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,15 @@ public void GetTypeCode_Invoke_ReturnsBoolean()
Assert.Equal(TypeCode.Char, 'a'.GetTypeCode());
}

[Fact]
public static void IsAscii_Char()
{
Assert.True(char.IsAscii(char.MinValue));
Assert.True(char.IsAscii('\x007f'));
Assert.False(char.IsAscii('\x0080'));
Assert.False(char.IsAscii(char.MaxValue));
Copy link
Member Author

@gfoidl gfoidl Aug 26, 2020

Choose a reason for hiding this comment

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

Is this enough for testing, or should be use the "whole range" ala

runtime/src/libraries/System.Runtime/tests/System/Text/RuneTests.TestData.cs

Line 233 in 6072e4d

public static IEnumerable<object[]> UnicodeInfoTestData_Latin1AndSelectOthers()

?

Copy link
Member

@danmoseley danmoseley Aug 26, 2020

Choose a reason for hiding this comment

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

The whole range. There is probably a similar test for Rune.IsAscii

Copy link
Member Author

@gfoidl gfoidl Aug 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Just scanned again through the repo, looking for IsAscii-tests. Haven't found one.

I've seen that all IsAscii implementations, like

runtime/src/libraries/System.Private.CoreLib/src/System/Text/Rune.cs

Line 143 in aa5fdab

public bool IsAscii => UnicodeUtility.IsAsciiCodePoint(_value);
delegate to

runtime/src/libraries/System.Private.CoreLib/src/System/Text/UnicodeUtility.cs

Line 120 in 6072e4d

public static bool IsAsciiCodePoint(uint value) => value <= 0x7Fu;

It's a trivial method (just one compare), but maybe it's better to delegate char.IsAscii also to this method and add tests over there (as I didn't find some for IsAsciiCodePoint -- maybe because it's defined in an internal type)?

Copy link
Member

@GrabYourPitchforks GrabYourPitchforks Aug 26, 2020

Choose a reason for hiding this comment

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

Search the file https://github.com/dotnet/runtime/blob/master/src/libraries/System.Runtime/tests/System/Text/RuneTests.cs for the substring "IsAscii". You'll see how we perform tests against the Rune APIs.

Though to be honest those tests are basically just testing specific values like '\u0000', '\u007f', etc., as you had here. :)

I have no problem with you leaving the test as you have it here.

}

[Fact]
public static void IsControl_Char()
{
Expand Down