-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Add Binary Parsing Support & Refactor TryGetNumberValue & ScanNumberHelper #7993
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.
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
Changes from all commits
2ef76c9
d9e8e09
76c1812
8628806
fc9e5cd
2b00b64
c7c129f
f737d30
3e92c0d
dd558a3
5bbe98f
bbf76c5
738c33e
4f68e43
52f7739
c537855
26248e6
dca6c6d
d59fd89
5eb6bf6
63ffd97
abc3e0b
5d34f9e
f358087
f66218d
1dd414e
8223a6a
e74c4a1
1c16a15
d03a4b9
8bbe0e3
b1b246c
cccd610
f555a2f
6b72b94
917af3d
169d687
1b52625
68f0a87
f7c16e5
6e1711b
5ecdf52
e08ad0b
270f6a0
ecc37bd
959e52c
99ec56d
091df16
684cc31
8822139
ec2e105
07649fb
c2c5e64
4f4cf22
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,17 +5,18 @@ | |||||||||||||||||
| using System.Collections.Concurrent; | ||||||||||||||||||
| using System.Collections.Generic; | ||||||||||||||||||
| using System.Collections.ObjectModel; | ||||||||||||||||||
| using System.ComponentModel; | ||||||||||||||||||
| using System.Diagnostics; | ||||||||||||||||||
| using System.Diagnostics.CodeAnalysis; | ||||||||||||||||||
| using System.Globalization; | ||||||||||||||||||
| using System.IO; | ||||||||||||||||||
| using System.Linq; | ||||||||||||||||||
| using System.ComponentModel; | ||||||||||||||||||
| using System.Management.Automation.Configuration; | ||||||||||||||||||
| using System.Management.Automation.Internal; | ||||||||||||||||||
| using System.Management.Automation.Language; | ||||||||||||||||||
| using System.Management.Automation.Runspaces; | ||||||||||||||||||
| using System.Management.Automation.Security; | ||||||||||||||||||
| using System.Numerics; | ||||||||||||||||||
| using System.Reflection; | ||||||||||||||||||
| using System.Runtime.CompilerServices; | ||||||||||||||||||
| using System.Runtime.InteropServices; | ||||||||||||||||||
|
|
@@ -38,6 +39,229 @@ namespace System.Management.Automation | |||||||||||||||||
| /// </summary> | ||||||||||||||||||
| internal static class Utils | ||||||||||||||||||
| { | ||||||||||||||||||
| /// <summary> | ||||||||||||||||||
| /// Converts a given double value to BigInteger via Math.Round(). | ||||||||||||||||||
| /// </summary> | ||||||||||||||||||
| /// <param name="d">The value to convert.</param> | ||||||||||||||||||
| /// <returns>Returns a BigInteger value equivalent to the input value rounded to nearest integer.</returns> | ||||||||||||||||||
| internal static BigInteger AsBigInt(this double d) => new BigInteger(Math.Round(d)); | ||||||||||||||||||
|
|
||||||||||||||||||
| internal static bool TryCast(BigInteger value, out byte b) | ||||||||||||||||||
| { | ||||||||||||||||||
| if (value < byte.MinValue || byte.MaxValue < value) | ||||||||||||||||||
| { | ||||||||||||||||||
| b = 0; | ||||||||||||||||||
| return false; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| b = (byte)value; | ||||||||||||||||||
| return true; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| internal static bool TryCast(BigInteger value, out sbyte sb) | ||||||||||||||||||
| { | ||||||||||||||||||
| if (value < sbyte.MinValue || sbyte.MaxValue < value) | ||||||||||||||||||
| { | ||||||||||||||||||
| sb = 0; | ||||||||||||||||||
| return false; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| sb = (sbyte)value; | ||||||||||||||||||
| return true; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| internal static bool TryCast(BigInteger value, out short s) | ||||||||||||||||||
| { | ||||||||||||||||||
| if (value < short.MinValue || short.MaxValue < value) | ||||||||||||||||||
| { | ||||||||||||||||||
| s = 0; | ||||||||||||||||||
| return false; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| s = (short)value; | ||||||||||||||||||
| return true; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| internal static bool TryCast(BigInteger value, out ushort us) | ||||||||||||||||||
| { | ||||||||||||||||||
| if (value < ushort.MinValue || ushort.MaxValue < value) | ||||||||||||||||||
| { | ||||||||||||||||||
| us = 0; | ||||||||||||||||||
| return false; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| us = (ushort)value; | ||||||||||||||||||
| return true; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| internal static bool TryCast(BigInteger value, out int i) | ||||||||||||||||||
| { | ||||||||||||||||||
| if (value < int.MinValue || int.MaxValue < value) | ||||||||||||||||||
| { | ||||||||||||||||||
| i = 0; | ||||||||||||||||||
| return false; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| i = (int)value; | ||||||||||||||||||
| return true; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| internal static bool TryCast(BigInteger value, out uint u) | ||||||||||||||||||
| { | ||||||||||||||||||
| if (value < uint.MinValue || uint.MaxValue < value) | ||||||||||||||||||
| { | ||||||||||||||||||
| u = 0; | ||||||||||||||||||
| return false; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| u = (uint)value; | ||||||||||||||||||
| return true; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| internal static bool TryCast(BigInteger value, out long l) | ||||||||||||||||||
| { | ||||||||||||||||||
| if (value < long.MinValue || long.MaxValue < value) | ||||||||||||||||||
| { | ||||||||||||||||||
| l = 0; | ||||||||||||||||||
| return false; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| l = (long)value; | ||||||||||||||||||
| return true; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| internal static bool TryCast(BigInteger value, out ulong ul) | ||||||||||||||||||
| { | ||||||||||||||||||
| if (value < ulong.MinValue || ulong.MaxValue < value) | ||||||||||||||||||
| { | ||||||||||||||||||
| ul = 0; | ||||||||||||||||||
| return false; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| ul = (ulong)value; | ||||||||||||||||||
| return true; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| internal static bool TryCast(BigInteger value, out decimal dm) | ||||||||||||||||||
| { | ||||||||||||||||||
| if (value < (BigInteger)decimal.MinValue || (BigInteger)decimal.MaxValue < value) | ||||||||||||||||||
| { | ||||||||||||||||||
| dm = 0; | ||||||||||||||||||
| return false; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| dm = (decimal)value; | ||||||||||||||||||
| return true; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| internal static bool TryCast(BigInteger value, out double db) | ||||||||||||||||||
| { | ||||||||||||||||||
| if (value < (BigInteger)double.MinValue || (BigInteger)double.MaxValue < value) | ||||||||||||||||||
| { | ||||||||||||||||||
| db = 0; | ||||||||||||||||||
| return false; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| db = (double)value; | ||||||||||||||||||
| return true; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// <summary> | ||||||||||||||||||
| /// Parses a given string or ReadOnlySpan<char> to calculate its value as a binary number. | ||||||||||||||||||
| /// Assumes input has already been sanitized and only contains zeroes (0) or ones (1). | ||||||||||||||||||
| /// </summary> | ||||||||||||||||||
| /// <param name="digits">Span or string of binary digits. Assumes all digits are either 1 or 0.</param> | ||||||||||||||||||
| /// <param name="unsigned"> | ||||||||||||||||||
| /// Whether to treat the number as unsigned. When false, respects established conventions | ||||||||||||||||||
| /// with sign bits for certain input string lengths. | ||||||||||||||||||
| /// </param> | ||||||||||||||||||
| /// <returns>Returns the value of the binary string as a BigInteger.</returns> | ||||||||||||||||||
| internal static BigInteger ParseBinary(ReadOnlySpan<char> digits, bool unsigned) | ||||||||||||||||||
| { | ||||||||||||||||||
| if (!unsigned) | ||||||||||||||||||
| { | ||||||||||||||||||
| if (digits[0] == '0') | ||||||||||||||||||
| { | ||||||||||||||||||
| unsigned = true; | ||||||||||||||||||
| } | ||||||||||||||||||
| else | ||||||||||||||||||
| { | ||||||||||||||||||
| switch (digits.Length) | ||||||||||||||||||
| { | ||||||||||||||||||
| // Only accept sign bits at these lengths: | ||||||||||||||||||
| case 8: // byte | ||||||||||||||||||
| case 16: // short | ||||||||||||||||||
| case 32: // int | ||||||||||||||||||
| case 64: // long | ||||||||||||||||||
| case 96: // decimal | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder - is this constants the same for all hardware platforms? Can we catch a problem on some ARMs? Or more general question - do we sure that PowerShell signed constants (binary and hex) is portable? @mklement0 have you any thoughts?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To my knowledge, there aren't any supported platforms where these numbers will be different. However, if there is a programmatic way to get expected sign bit lengths, I would be more than open to checking against this instead.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vexx32 It is not low level problem because we say about
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aye, that could definitely be a concern. I could always refactor this to incorporate this, but I'd need to know what to incorporate here to account for it. Not sure where to find this information at the moment. 🙁
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found nothing useful. I hope @mklement0 help.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vexx32: The always-positive logic is fairly easy to implement, if I understand things correctly: From a hex string: prepend a // 0xff as 255; without the leading 0, the result would be -1
BigInteger.Parse("0ff",NumberStyles.AllowHexSpecifier)From a byte array - prepend a new BigInteger(new byte[] { 0x0, 0xff }, isBigEndian: true)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even easier, actually. Hex strings are currently given a leading zero by default with my current code. I am then stripping it out in cases where it appears appropriate to allow a sign bit. I would simply remove the additional logic. And with binary, it would simply always mean invoking the BigInteger constructor with a value of
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking further, I could even trim out the extra leading 0 if we eventually opt for parsing as unsigned, as it will be possible to simply instruct the BigInteger constructor to always read it as unsigned.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for the delay, on the topic of the breaking change. I know it's desirable to change it, but it seems a bit risky to make such a change.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I figured that would probably be the case. It is a fairly straightforward follow up if we find reason to change it later. I could even put it in as an experimental feature later on, I think. |
||||||||||||||||||
| case int n when n >= 128: // BigInteger | ||||||||||||||||||
| break; | ||||||||||||||||||
| default: | ||||||||||||||||||
| // If we do not flag these as unsigned, bigint assumes a sign bit for any (8 * n) string length | ||||||||||||||||||
| unsigned = true; | ||||||||||||||||||
| break; | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // Only use heap allocation for very large numbers | ||||||||||||||||||
| const int MaxStackAllocation = 512; | ||||||||||||||||||
|
|
||||||||||||||||||
| // Calculate number of 8-bit bytes needed to hold the input, rounded up to next whole number. | ||||||||||||||||||
| int outputByteCount = (digits.Length + 7) / 8; | ||||||||||||||||||
| Span<byte> outputBytes = outputByteCount <= MaxStackAllocation ? stackalloc byte[outputByteCount] : new byte[outputByteCount]; | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CoreFX team recommend to use a const for stack allocations to get more fast code.
Suggested change
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did give this a look, but this completely breaks parsing due to it typically filling the bytes from first to last, and the actual BigInteger constructor being used as big-endian (it ends up parsing every number as at least a 512-byte number, and with the method of filling bytes it currently uses, this means it fills the high byte instead of the low byte). This could be worked in, I think, but I think it would require additional allocations that I think it would likely be more effective to avoid.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @iSazonov once I got the logic fixed, which wasn't nearly as hard as I first thought, here are the results of two different methods of doing it vs the current method. The first alternate method slices the over-allocated span down to size before passing it to the BigInteger constructor, and the second simply ensures that all bytes are positioned in the span to be correctly parsed, which just meant redefining the starting point to be the end of the span, instead of counting the bytes needed there. https://gist.github.com/vexx32/375d5553dfe3edee60cd9616228dd3a5 In this instance, it seems that even allocating according to a
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using const speeds up calling the method not the method itself. Are you sure that your test measure this correctly?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if it speeds up calling the method, the excess allocation is taking far more time than it could possibly be saving on the method call, unless method calls are an order of magnitude more expensive than I am generally led to believe. But be that as it may, I'm not really sure how one would measure that appropriately.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I was not accurate. The compiler optimization can be performed only if
Suggested change
I'm ok with current code too.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, interesting, thank you! I'll see if that makes an appreciable difference. 😄
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @iSazonov I'm not sure what kind of perf benefit this should give, but all my attempts to include this suggestion haven't seemed to produce the results you're describing. https://gist.github.com/vexx32/d2b7fff82f93635ea974bdbc0c26fa89 This is the benchmarking code I'm using, with BenchmarkDotNet targeting Thank you for all your fantastic help! 😄 |
||||||||||||||||||
| int outputByteIndex = outputBytes.Length - 1; | ||||||||||||||||||
|
|
||||||||||||||||||
| // We need to be prepared for any partial leading bytes, (e.g., 010|00000011|00101100), or cases | ||||||||||||||||||
| // where we only have less than 8 bits to work with from the beginning. | ||||||||||||||||||
| // | ||||||||||||||||||
| // Walk bytes right to left, stepping one whole byte at a time (if there are any whole bytes). | ||||||||||||||||||
| int byteWalker; | ||||||||||||||||||
| for (byteWalker = digits.Length - 1; byteWalker >= 7; byteWalker -= 8) | ||||||||||||||||||
| { | ||||||||||||||||||
| // Use bit shifts and binary-or to sum the values in each byte. These calculations will | ||||||||||||||||||
| // create values higher than a single byte, but the higher bits will be stripped out when cast | ||||||||||||||||||
| // to byte. | ||||||||||||||||||
| // | ||||||||||||||||||
| // The low bits are added in separately to allow us to strip the higher 'noise' bits before we | ||||||||||||||||||
| // sum the values using binary-or. | ||||||||||||||||||
| // | ||||||||||||||||||
| // Simplified representation of logic: (byte)( (7)|(6)|(5)|(4) ) | ( ( (3)|(2)|(1)|(0) ) & 0b1111 ) | ||||||||||||||||||
| // | ||||||||||||||||||
| // N.B.: This code has been tested against a straight for loop iterating through the byte, and in no | ||||||||||||||||||
| // circumstance was it faster or more effective than this unrolled version. | ||||||||||||||||||
| outputBytes[outputByteIndex--] = | ||||||||||||||||||
| (byte)( | ||||||||||||||||||
| ( (digits[byteWalker - 7] << 7) | ||||||||||||||||||
| | (digits[byteWalker - 6] << 6) | ||||||||||||||||||
| | (digits[byteWalker - 5] << 5) | ||||||||||||||||||
| | (digits[byteWalker - 4] << 4) | ||||||||||||||||||
| ) | ||||||||||||||||||
| | ( | ||||||||||||||||||
| ( (digits[byteWalker - 3] << 3) | ||||||||||||||||||
| | (digits[byteWalker - 2] << 2) | ||||||||||||||||||
| | (digits[byteWalker - 1] << 1) | ||||||||||||||||||
| | (digits[byteWalker]) | ||||||||||||||||||
| ) & 0b1111 | ||||||||||||||||||
| ) | ||||||||||||||||||
| ); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // With complete bytes parsed, byteWalker is either at the partial byte start index, or at -1 | ||||||||||||||||||
| if (byteWalker >= 0) | ||||||||||||||||||
| { | ||||||||||||||||||
| int currentByteValue = 0; | ||||||||||||||||||
| for (int i = 0; i <= byteWalker; i++) | ||||||||||||||||||
| { | ||||||||||||||||||
| currentByteValue = (currentByteValue << 1) | (digits[i] - '0'); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| outputBytes[outputByteIndex] = (byte)currentByteValue; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| return new BigInteger(outputBytes, isUnsigned: unsigned, isBigEndian: true); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // From System.Web.Util.HashCodeCombiner | ||||||||||||||||||
| internal static int CombineHashCodes(int h1, int h2) | ||||||||||||||||||
| { | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,39 +27,64 @@ internal static class SpecialChars | |
| [Flags] | ||
| internal enum CharTraits | ||
| { | ||
| /// <summary> | ||
| /// No specific character traits. | ||
| /// </summary> | ||
| None = 0x0000, | ||
|
|
||
| // For identifiers, is the character a letter? | ||
| /// <summary> | ||
| /// For identifiers, the first character must be a letter or underscore. | ||
| /// </summary> | ||
| IdentifierStart = 0x0002, | ||
|
|
||
| // The character is a valid first character of a multiplier | ||
| /// <summary> | ||
| /// The character is a valid first character of a multiplier. | ||
| /// </summary> | ||
| MultiplierStart = 0x0004, | ||
|
|
||
| // The character is a valid type suffix for numeric literals | ||
| /// <summary> | ||
| /// The character is a valid type suffix for numeric literals. | ||
| /// </summary> | ||
| TypeSuffix = 0x0008, | ||
|
|
||
| // The character is a whitespace character | ||
| /// <summary> | ||
| /// The character is a whitespace character. | ||
| /// </summary> | ||
| Whitespace = 0x0010, | ||
|
|
||
| // The character terminates a line. | ||
| /// <summary> | ||
| /// The character terminates a line. | ||
| /// </summary> | ||
| Newline = 0x0020, | ||
|
|
||
| // The character is a hexadecimal digit. | ||
| /// <summary> | ||
| /// The character is a hexadecimal digit. | ||
| /// </summary> | ||
| HexDigit = 0x0040, | ||
|
|
||
| // The character is a decimal digit. | ||
| /// <summary> | ||
| /// The character is a decimal digit. | ||
| /// </summary> | ||
| Digit = 0x0080, | ||
|
|
||
| // The character is allowed as the first character in an unbraced variable name. | ||
| /// <summary> | ||
| /// The character is allowed as the first character in an unbraced variable name. | ||
| /// </summary> | ||
| VarNameFirst = 0x0100, | ||
|
|
||
| // The character is not part of the token being scanned. | ||
| /// <summary> | ||
| /// The character is not part of the token being scanned. | ||
| /// </summary> | ||
| ForceStartNewToken = 0x0200, | ||
|
|
||
| // The character is not part of the token being scanned, when the token is known to be part of an assembly name. | ||
| /// <summary> | ||
| /// The character is not part of the token being scanned, when the token is known to be part of an assembly name. | ||
| /// </summary> | ||
| ForceStartNewAssemblyNameSpecToken = 0x0400, | ||
|
|
||
| // The character is the first character of some operator (and hence is not part of a token that starts a number) | ||
| /// <summary> | ||
| /// The character is the first character of some operator (and hence is not part of a token that starts a number). | ||
| /// </summary> | ||
| ForceStartNewTokenAfterNumber = 0x0800, | ||
| } | ||
|
|
||
|
|
@@ -150,7 +175,7 @@ static CharExtensions() | |
| /* K */ CharTraits.IdentifierStart | CharTraits.VarNameFirst | CharTraits.MultiplierStart, | ||
| /* L */ CharTraits.IdentifierStart | CharTraits.VarNameFirst | CharTraits.TypeSuffix, | ||
| /* M */ CharTraits.IdentifierStart | CharTraits.VarNameFirst | CharTraits.MultiplierStart, | ||
| /* N */ CharTraits.IdentifierStart | CharTraits.VarNameFirst, | ||
| /* N */ CharTraits.IdentifierStart | CharTraits.VarNameFirst | CharTraits.TypeSuffix, | ||
| /* O */ CharTraits.IdentifierStart | CharTraits.VarNameFirst, | ||
| /* P */ CharTraits.IdentifierStart | CharTraits.VarNameFirst | CharTraits.MultiplierStart, | ||
| /* Q */ CharTraits.IdentifierStart | CharTraits.VarNameFirst, | ||
|
|
@@ -182,7 +207,7 @@ static CharExtensions() | |
| /* k */ CharTraits.IdentifierStart | CharTraits.VarNameFirst | CharTraits.MultiplierStart, | ||
| /* l */ CharTraits.IdentifierStart | CharTraits.VarNameFirst | CharTraits.TypeSuffix, | ||
| /* m */ CharTraits.IdentifierStart | CharTraits.VarNameFirst | CharTraits.MultiplierStart, | ||
| /* n */ CharTraits.IdentifierStart | CharTraits.VarNameFirst, | ||
| /* n */ CharTraits.IdentifierStart | CharTraits.VarNameFirst | CharTraits.TypeSuffix, | ||
| /* o */ CharTraits.IdentifierStart | CharTraits.VarNameFirst, | ||
| /* p */ CharTraits.IdentifierStart | CharTraits.VarNameFirst | CharTraits.MultiplierStart, | ||
| /* q */ CharTraits.IdentifierStart | CharTraits.VarNameFirst, | ||
|
|
@@ -298,18 +323,17 @@ internal static bool IsHexDigit(this char c) | |
| return false; | ||
| } | ||
|
|
||
| // Return true if the character is a decimal digit. | ||
| internal static bool IsDecimalDigit(this char c) | ||
| { | ||
| if (c < 128) | ||
| { | ||
| return (s_traits[c] & CharTraits.Digit) != 0; | ||
| } | ||
| // Returns true if the character is a decimal digit. | ||
| internal static bool IsDecimalDigit(this char c) => (uint)(c - '0') <= 9; | ||
|
|
||
| return false; | ||
| } | ||
| // These decimal/binary checking methods are more performant than the alternatives due to requiring | ||
| // less overall operations than a more readable check such as {(this char c) => c == 0 | c == 1}, | ||
| // especially in the case of IsDecimalDigit(). | ||
|
|
||
| // Returns true if the character is a binary digit. | ||
| internal static bool IsBinaryDigit(this char c) => (uint)(c - '0') <= 1; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While this is true, it seems more straightforward to just have
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Perhaps the coercion + comparison has a performance advantage?)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rjmholt I was following @iSazonov's lead on that one, he apparently pulled that from the CoreFX code somewhere or other. Knowing nothing about how these checks would be performed on a hardware level means I can only speculate on potential ways this might be better than the ultimately far more readable version you propose. Should I change it?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rjmholt The micro optimization come from CoreFX - 2 vs 3 operations - this gives huge perf win.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should I add a comment to this effect?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we get the question the comment can help code readers. |
||
|
|
||
| // Return true if the character is a type suffix character. | ||
| // Returns true if the character is a type suffix character. | ||
| internal static bool IsTypeSuffix(this char c) | ||
| { | ||
| if (c < 128) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.