Correctly show the '}', ']' within the exception message in Utf8JsonWriter

[ahsonkhan]

Before:
'0' is invalid without a matching open.

Now:
']' is invalid without a matching open.

Was previously passing the token value as currentDepth so the default byte value (0) was being displayed. Using named parameters fixes that.

[stephentoub]

Using named parameters fixes that.

This seems like a pretty easy mistake to make, given all the optional parameters. Have you considered just not making them optional?

[ahsonkhan]

This seems like a pretty easy mistake to make, given all the optional parameters. Have you considered just not making them optional?

At all the call sites, I ThrowInvalidOperationException, I am passing one of the optional arguments, all of which get passed to GetResourceString.

I could have three overloads instead.

public static void ThrowInvalidOperationException(ExceptionResource resource, int currentDepth)
{
    throw GetInvalidOperationException(resource, currentDepth, default, default);
}

public static void ThrowInvalidOperationException(ExceptionResource resource, byte token)
{
    throw GetInvalidOperationException(resource, default, token, default);
}

public static void ThrowInvalidOperationException(ExceptionResource resource, JsonTokenType tokenType)
{
    throw GetInvalidOperationException(resource, default, default, tokenType);
}

[stephentoub]

I am passing one of the optional arguments

Why not just pass all three? It can't be a perf argument (compared to the optional arguments), as the IL will be identical. (There would be an IL difference for the three methods, obviously... if that's the motivation and it actually matters, then sure, go for the three different methods.)

[ahsonkhan]

Why not just pass all three?

And pass in just default values for the ones I don't have in the context of the method (for instance, I don't always have token)? The other two are fields so I could always pass those around (_currentDepth, _tokenType).

It can't be a perf argument, as the IL will be identical.

I don't get what you mean here. Passing in more (unnecessary) arguments doesn't affect the IL?

[stephentoub]

I don't get what you mean here. Passing in more (unnecessary) arguments doesn't affect the IL?

If I have a method:

Foo(int a = 1, int b = 2) { … }

and I have two call sites:

Foo();

and

Foo(1, 2);

the IL for both will be identical, both of them loading 1 and 2 and passing them into Foo. The optional arguments aren't actually optional at the IL level: the compiler burns those values in at the call site.

[ahsonkhan]

The optional arguments aren't actually optional at the IL level: the compiler burns those values in at the call site.

Ah I see what you mean. That makes sense.

However, if you didn't have optional arguments.

Foo() { … }
Foo(int a, int b) { … }

Now the two call sites would be different, correct?

Foo()

and

Foo(1, 2)

[stephentoub]

However, if you didn't have optional arguments. ... Now the two call sites would be different, correct?

Correct.

[ahsonkhan]

Looks like the disassembly and IL is affected though (creating stack space - push/pop). At least for netfx:
sharplab.io

// Code size 29 (0x1d)
vs 
// Code size 24 (0x18)

[stephentoub]

Looks like the disassembly and IL is affected though

You're not doing the right comparison: the right comparison is passing in the default values.

[ahsonkhan]

You're not doing the right comparison: the right comparison is passing in the default values.

Got it. That makes sense now :)

ThrowInvalidOperationException(_currentDepth, default, default);

sharplab.io