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

[CBOR] Implement indefinite length writer and reader support #33831

Merged
merged 12 commits into from
Mar 24, 2020

Conversation

eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Mar 19, 2020

Adds indefinite length writer and reader support for strings, maps and arrays:

  • Implement write support.
  • Implement read support.

@eiriktsarpalis eiriktsarpalis marked this pull request as ready for review March 22, 2020 00:49
// performing validation and returning a list of ranges containing the individual chunk payloads
private List<(int offset, int length)> ReadChunkedStringRanges(CborMajorType type, out int encodingLength, out int concatenatedBufferSize)
{
var ranges = new List<(int offset, int length)>();
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if it's worth the complexity; but you could do something like

List<(int offset, int length)> ranges = Interlocked.Exchange(ref _ranges, null);
ranges ??= new List<(int, int)>();
ranges.Clear();

And have all of the calling members assign it back to the field right before returning.

It's not super important, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll give it a shot, provided it doesn't make things too hard to read. Or maybe wait until benchmarking demonstrates issues with allocations.

Also, why Interlocked? The class offers no thread safety guarantees.

Copy link
Member

Choose a reason for hiding this comment

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

It's both a quick way of "I've taken this" (in case somehow this ever went recursive, which I know isn't currently possible) and it means that the kinds of scary race conditions are limited. (overlapping writes from a read method is scarier to me than read twice or read slightly forward)

@eiriktsarpalis eiriktsarpalis merged commit 5c8cad2 into dotnet:master Mar 24, 2020
@eiriktsarpalis eiriktsarpalis deleted the feature/cbor branch March 24, 2020 22:01
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants