Can the IPAddress(byte[], long) ctor be implemented in terms of this new one?

public IPAddress(byte[] address, long scopeid) :
    this(new ReadOnlySpan<byte>(address ?? new ArgumentNullException(nameof(address))), scopeid)
{
}

or does that negatively impact the perf of the byte[]-based ctor?

I didn't think of throwing the exception that way. Started with sharing the code but then the null check had to be specifically in the array based ctor and I ended up with the copy pasting of code.

Will change both ctors to use this type of null check. Thanks!

Same question; is there a reason we can't implement the existing byte[]-based ctor in terms of the span-based one?

Why is this check needed?

Leftover from troubleshooting that snuck in. I'll get rid of it.

Nit: I know the existing code doesn't have this, but it'd be nice to name the Boolean argument at the call site; otherwise it's difficult to know while reading the code what it means. (Also the parens around the expression below are unnecessary... again, I realize you were just copying the existing style.)

Nit: for robustness, it'd be good to assert that this returns true, e.g.

bool success = TryWriteBytes(new Span<byte>(bytes), out int bytesWritten);
Debug.Assert(success);
return bytes;

This code could be cleaned up while you're at it, and simplified to just:

int length = IsIPv6 ? IPAddressParserStatics.IPv6AddressBytes : IPAddressParserStatics.IPv4AddressBytes;
var bytes = new byte[length];

Nit: you might condense this slightly, e.g.

public bool TryFormat(Span<char> destination, out int charsWritten) =>
    IsIPv4 ?
        IPAddressParser.IPv4AddressToString(PrivateAddress, destination, out charsWritten) :
        IPAddressParser.IPv6AddressToString(_numbers, PrivateScopeId, destination, out charsWritten);

Could this be written in terms of ReadOnlySpan<char> rather than in terms of char*? That'd a) be safer and b) avoid pinning for unnecessary periods of time. Regardless, the null check below can be moved up into the string-based overload.

If you do switch to using span instead of pointers, you shouldn't need this custom ContainsColon either, as you should be able to just use Span.IndexOf. At the moment that'll require a reference to System.Memory, but hopefully that won't be needed in the near future.

Any idea of which way to go? Both will require changes once the reference isn't required anymore.

Especially in networking-related code, we want to avoid unnecessary unsafe code, so it'd be great to go the route of using spans as much as possible rather than introducing additional pointer-based code. For now I'd minimize changing pointer-based code to spans, as it could have more perf impact right now than we want to accept, but when converting string-based methods to something else, let's prefer spans over pointers.

Seems like this method should be made a private helper, and then another internal wrapper for it should be added that does this check. It's not needed when called from the string-returning version, only for the span one.

Is it worth the extra method call to avoid that branch instruction? I think it might be better this way but I'm open to trying to measure it.

Why build up into a StringBuilder only to then copy the data out? We can't just go into the destination span directly?

I went for this design to keep as much as possible shared. I will look into allocating a fixed size buffer for the string version instead.

I see: the existing code is already using a StringBuilder, and you're now just passing that out of the call. Ok, makes sense. After this PR, it might make sense to look at cleaning that up further; seems like we should be able to avoid the StringBuilder entirely, but for now it's fine.

Nit: please change var => StringBuilder.

Should I avoid using var in general for corefx repo?

No, var is ok, but only if the type is named on the right-hand side, i.e. with a new or cast expression. We want the type of the variable to be easily understood by someone reading the code.

This is now accessing a ref inside of a loop. That's potentially expensive.

Didn't know that about refs. Will fix.

It just inhibits certain optimizations, as the compiler doesn't necessarily know whether the target is changing between uses.

Using a local copy instead.

I'm not understanding why this function needed to change...?

Previous version wrote the IP from the end of the buffer. Writing this directly into the span caused leading \0. Changed it to write from the start of the buffer.

I see. Ok. In that case it'd be worth adding an assert at the beginning validating that number <= 255. Or better yet, make it a byte rather than an int.

Went with the assert as Math.DivRem only have overloads for int and long

As earlier, it'd be good to using ReadOnlySpan<char> instead of the original string, rather than substituting char*. Especially in this IPAddress code, it'd be great to try to avoid adding any unnecessary unsafe code.

Ditto on using ReadOnlySpan<char> unless there's a strong reason to avoid it. Will stop commenting on this, but it applies throughout the code you're touching... let's try to only switch to char* when calling existing functions that accept it.

What was wrong with the code in this file as it was?

The previous behaviour failed to build with ambigous overload error. Forcing the null to be of string type eliminates the Span version from the available candidates.

Ok. FWIW, you can do that just by adding a cast, e.g. (string)null.

It's great to see lots of tests added, but a lot of these look like duplication of existing tests, just for the span-based overloads instead of the array-based ones. It'd be good to find a way to avoid the duplication and reuse tests once for both the array-based and span-based APIs. There are a variety of approaches to this, but one example is here:
https://github.com/dotnet/corefx/blob/master/src/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/EncryptDecrypt.cs
The tests are all in an abstract base class that exposes virtuals for each of the APIs that has an array vs span variant, and then derived types just override those methods, providing functionality to the base class to be tested.

I should have been clearer about naming the argument. I meant doing it like this:

address = IPAddressParser.Parse(ipString, tryParse: true)

Ditto.

Ditto.

Rather than writing this method, you should be able to add a reference to System.Memory in the .csproj:
https://github.com/dotnet/corefx/blob/master/src/System.Net.Primitives/src/System.Net.Primitives.csproj#L200
Then you can delete this method, and at the call site instead of doing:

if (ContainsColon(ipSpan))

do:

if (ipSpan.IndexOf(':') >= 0)

That not only saves the extra code, but this will benefit from any perf improvements over the standard loop done in IndexOf.

Ouch, we should not be allocating a char[] from the string. Once you've added the System.Memory reference, you can use the AsReadOnlySpan extension method.

This no longer needs to be unsafe.

Is this a typo? Won't this call itself recursively and stack overflow?

This seems to have worked by accident because of the parameters. Renaming it to avoid issues.

Nit: unnecessary blank line

This no longer needs to use a pointer. The pointer was here due to stackallocating, but now we can just write this in terms of Span<char>, making this method safer against possible overflow bugs. The span-based caller can just pass in the target span. And the string-based caller can still stackalloc the target space, but then rather than passing that in directly, it can create a span from it and pass in the span here. That then also avoids the pinning that's been added in the span-based caller above.

Could we add an assert that verifies nothing was written to the output array? e.g.

Assert.Equal<byte>(new char[result.Length], result);

Why would valid addresses fail to format? Too little destination space? If so, can we include that in the title?

Assuming this is the case, my suggestion would be to combine the success and failure tests for this, and validate three things:

• That it succeeds with exactly the right amount of output space
• That it fails with one less byte available than is needed, and none of the output is written
• That it succeeds with one more byte available than is needed, and that extra byte remains unwritten

You could do a similar thing with:

Assert.Equal<byte>(
    expected.AsSpan().Slice(0, length).ToArray(),
    actual.AsSpan().Slice(0, length).ToArray());

Is there a better way to do this than to cast to void*. Would having a T* overload for Span collide with the void* version?

You shouldn't need to explicitly cast: a char* is implicitly convertible to a void*.

ps The reason it's void* is because you can't have a T* in C#.

ahh, thanks!

Nit: please add it in alphabetical order