Skip to content
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

Add support for using TypeId as dictionary keys with system.text.json #17

Merged
merged 2 commits into from
Apr 14, 2024
Merged

Add support for using TypeId as dictionary keys with system.text.json #17

merged 2 commits into from
Apr 14, 2024

Conversation

danspam
Copy link
Contributor

@danspam danspam commented Feb 21, 2024

See #16

Copy link
Owner

@firenero firenero left a comment

Choose a reason for hiding this comment

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

Thanks for submitting the PR and covering it with tests 👍
The change looks good overall. I only suggested some minor improvements, let me know what you think about those.

@@ -15,4 +16,15 @@ public override void Write(Utf8JsonWriter writer, TypeId value, JsonSerializerOp
{
writer.WriteStringValue(value.ToString());
}

public override void WriteAsPropertyName(Utf8JsonWriter writer, [DisallowNull] TypeId value, JsonSerializerOptions options)
Copy link
Owner

Choose a reason for hiding this comment

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

[DisallowNull] attribute is redundant here because TypeId is a struct and can't be null. I'd suggest removing it with using System.Diagnostics.CodeAnalysis.

return ReadTypeId(ref reader);
}

public override void WriteAsPropertyName(Utf8JsonWriter writer, [DisallowNull] TypeIdDecoded value, JsonSerializerOptions options)
Copy link
Owner

Choose a reason for hiding this comment

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

Similarly to TypeId, [DisallowNull] attribute is redundant here because TypeIdDecoded is a struct and can't be null. I'd suggest removing it with using System.Diagnostics.CodeAnalysis.

return val is not null ? TypeId.Parse(val).Decode() : default;
}

private static void CopyValueToBuffer(TypeIdDecoded value, Span<char> buffer)
Copy link
Owner

Choose a reason for hiding this comment

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

To avoid producing copies of parameters let's consider using in modifier. Since both are greater or equal to pointer size it should be more performant than passing a copy.

Suggested change
private static void CopyValueToBuffer(TypeIdDecoded value, Span<char> buffer)
private static void CopyValueToBuffer(in TypeIdDecoded value, in Span<char> buffer)

Copy link
Owner

Choose a reason for hiding this comment

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

Benchmarked all options here and it turns out that it only makes sense to use in for TypeIdDecoded value parameter.

@firenero
Copy link
Owner

@danspam I've decided to remove redundant attributes myself to get this PR merged. Thanks again for your great contribution!

@firenero firenero merged commit a92d92f into firenero:main Apr 14, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants