It is clear logic for sign but I think we need more feedback whether this is a good UX.

This is consistent with the existing patterns for things like hexadecimal, but I would be inclined to agree that there could be improvements made here, should we want to.

What about:

return c & 0x30 || c & 0x31;

There's no reason we can't do this, but I don't see anything else in the file that behaves this way and felt it best to continue the prevalent pattern here?

+30% performance :-)

Based on the experiment below my suggestion should be faster :-)

Do you meant c == 0x30 || c == 0x31? & does not return a straight true/false or 0 / 1
(we can only do this in the binary parse because we also & 0b1111 or strip the high bits by cast to (byte))

@iSazonov honestly a lot of these could also be reformatted like this regardless. For example:

internal static bool IsDecimalDigit(this char c)
{
    if (c < 128)
    {
        return (s_traits[c] & CharTraits.Digit) != 0;
    }

    return false;
}

becomes

internal static bool IsDecimalDigit(this char c) => c < 128 && (s_traits[c] & CharTraits.Digit) != 0;

Is this a worthwhile change to make to the common pattern of these methods?

Yes, I think we can do this.
Pattern from CoreFX repo:

internal static bool IsBinaryDigit(this char c) => (uint)(c - '0') <= 1;
internal static bool IsDecimalDigit(this char c) => (uint)(c - '0') <= 9;

Done! :)

Why we use auto-decrement here? I'd prefer do "-1" in previous init line.

It just felt clever to use autodecrement to change the variable from "size of array" to "last index of array".
It is not very clear, and could be outputByteWalker -= 1 in the following line, if that's more readable. Or like this?

            Span<byte> outputBytes = new byte[(digits.Length + 7) / 8];
            int outputByteWalker = outputBytes.Length - 1;

We could use more informational name like outputByteWalkerLastIndex.

Please don't use outputByteWalker-- here - very bad readability is way to bugs.
I'd add new local variable - one for size/length, second for last index if needed.

I'll take care of this momentarily. 😄

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?

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.

@vexx32 It is not low level problem because we say about digits - it can be PowerShell language propblem if ARM users expect sign bit on another position.

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. 🙁

I found nothing useful. I hope @mklement0 help.

@vexx32: The always-positive logic is fairly easy to implement, if I understand things correctly:

From a hex string: prepend a 0:

// 0xff as 255; without the leading 0, the result would be -1
BigInteger.Parse("0ff",NumberStyles.AllowHexSpecifier)

From a byte array - prepend a 0x byte (with Big-Endian ordering):

new BigInteger(new byte[] { 0x0, 0xff }, isBigEndian: true)

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 true for the isUnsigned parameter.

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.

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.

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.

Why we use the extra parentheses?

It lets us make (8x zero + 7x OR) operations down to (1x zeroing + 8x OR), and is faster.

The brackets are:

(byte) (
    ((7)|(6)|(5)|(4))   |  ( ((3)|(2)|(1)|0) & 0b1111 )
)

This works because the bit-patterns in the input (chars '0', '1') are 00110000 and 00110001. That is a coincidence of them being ASCII 48, ASCII 49. We can take advantage of those bit patterns, and do less work. We could zero the 0011 on every character, like this more readable slower code, and then shift the least significant bit, and OR them all together:

(byte)(
      ((digits[byteWalker - 7] - '0') << 7) |
      ((digits[byteWalker - 6] - '0') << 6) |
      ((digits[byteWalker - 5] - '0') << 5) |
      ((digits[byteWalker - 4] - '0') << 4) |
      ((digits[byteWalker - 3] - '0') << 3) |
      ((digits[byteWalker - 2] - '0') << 2) |
      ((digits[byteWalker - 1] - '0') << 1) |
      ((digits[byteWalker - 0] - '0')     )
      )

But, for the 7,6,5,4 shifts, they will push the 0011 part far out of the way. When we cast to (byte) they get dropped. Saves 4x zeroing operations if we treat those as a group.

The 3,2,1,0 shifts will combine with each other, but the high 0011 bits would clash if we weren't careful. So we merge these as a group, then do a single & 0b1111 of this group, and save 3x zeroing.

Then +1x OR to merge the two halves. ((7)|(6)|(5)|(4)) | ( ((3)|(2)|(1)|0) & 0b1111 )

This is quite hard to explain, I hope this is readable.

Why not ( (7)|(6)|(5)|(4)|(3)|(2)|(1)|(0) ) & 0b1111?

That would not give the right answer, it would make the high bits from inputs 3,2,1,0 overwrite the data from 7,6,5,4 (then & 0b1111 would leave only 0-15 output values).

String input:        "10101011"
index from right:     76543210

Bit patterns of shifted input characters, 
we want just the single far right bit from each line, 
all merged down:

    0011000|7
     001100|06
      00110|005   
       0011|0004
             |-------possible overlap when combining
        001|1000|3
         00|1100|02
          0|0110|001
           |0011|0000

             ^ removed by &0b1111 to prevent overlap
                only in the 3,2,1,0 section

      ^ far left overlap is OK, dropped by (byte) cast

Thanks for clarification!

I think we need reformat the code and add the comment.

I would appreciate some suggestions on how to approach the reformatting. The actual behaviour of the code is documented in the comments (although perhaps that can also be improved upon).

I suspect it will be necessary to skirt StyleCop rules heavily to make this much more readable than it is?

1. We could add the well-formatted sample ( (7)|(6)|(5)|(4) ) | ( ( (3)|(2)|(1)|(0) ) & 0b1111 ) in your comment.
2. We could add white spaces in code for better readability.

Sounds good!

I think this might annoy StyleCop, but... such is the price of readability. 😄

While this is true, it seems more straightforward to just have => c == '0' || c == '1';. It took me a minute or so to realise why this isn't a problem.

(Perhaps the coercion + comparison has a performance advantage?)

@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?

@rjmholt The micro optimization come from CoreFX - 2 vs 3 operations - this gives huge perf win.

Should I add a comment to this effect?

Because we get the question the comment can help code readers.

No need for the else here; following the branch above always returns