Skip to content
This repository has been archived by the owner on Aug 2, 2023. It is now read-only.

Remove ReadOnlyBytes #2044

Merged
merged 1 commit into from
Jan 11, 2018
Merged

Conversation

KrzysztofCwalina
Copy link
Member

@KrzysztofCwalina KrzysztofCwalina commented Jan 10, 2018

<edit>
I just moved ISlicable to Experimental package and changed the tests to use an adapter. A long term solution would mean ReadOnlyBytes implementing ISlicable, but for now I want to merge this PR to remove ReadOnlyBytes
</edit>

This PR shows what it would take to remove ReadOnlyBytes and still support TryReadUntil on BufferReader. TryReadUntill reads till some delimiter and then returns the bytes read. The difference between TryReadUntill and all the other "read" methods we already support is that its result is not fixed-size, i.e. the result is ReadOnlyBytes itself. Other read methods return ints, floats, datetimes, etc.

To summarize:

  1. We would need to add ISlicable<T> interface. If BufferReader wraps a type implementing ISlicable, the reader would support TryReadUntil; otherwise TryReadUntil would not be available (the extension constraint would not be fulfilled).
  2. We would need to expose Sequence property on BufferReader.

It works, but I wish it all was simpler.

cc: @pakrym , @davidfowl, @joshfree, @terrajobst, @halter73, @ahsonkhan

BTW, I assume/assert we must support something like TryReadUntil on the reader, i.e. our abstractions are not shippable if we cannot make it work.

Other alternatives we explored (and my commentary on them):

  1. Make new ReadOnlyBytes(Position start, Position end) work. It's not clear this is possible without severely constraining what can be done with Position, but maybe it's worth trying.
  2. Change BufferReder to be specific to ReadOnlyBuffer, as opposed to being over ISequence. I worry that this will result in less APIs being developed that operate in buffer sequences. With ISequence, I imagine we will develop many transportation and parsing APIs (e.g. compress sequence, parse JSON, etc. ) that operate on ISequence. I worry that ReadOnlyBuffer is too policy heavy to embark on major effort to develop all these APIs that would work just on it.
  3. Have TryReadUntil to always incur a data copy. I think this goes too much against our philosophy of non-alloc.

Any other ideas?

@ahsonkhan
Copy link
Member

cc @pakrym (not Pranav)

@KrzysztofCwalina
Copy link
Member Author

I just moved ISlicable to Experimental package and changed the tests to use an adapter. A long term solution would mean ReadOnlyBytes implementing ISlicable, but for now I want to merge this PR to remove ReadOnlyBytes completly.

@pakrym, are you ok with this merge?

Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

Get rid of the commenter our code

}
}
}
//// Copyright (c) Microsoft. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this commented out?

Choose a reason for hiding this comment

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

@davidfowl, Sarah Faraphallower said it wasn't required...

Choose a reason for hiding this comment

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

Won't immediately say I disagree either, my 3 Sat and CRT/JMT logic tables work without it....

Copy link
Member Author

Choose a reason for hiding this comment

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

@davidfowl, something went very wrong with the merge. I really don't understand how the tests passed. Anyway, I meant to delete this whole file, and I will shortly.

@KrzysztofCwalina
Copy link
Member Author

@dotnet-bot test Innerloop Ubuntu16.04 Debug Build and Test

Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

Not sure why there's a conflict but this LGTM

@KrzysztofCwalina
Copy link
Member Author

@dotnet-bot test Innerloop Ubuntu16.04 Release Build and Test

@KrzysztofCwalina KrzysztofCwalina merged commit 57abf68 into dotnet:master Jan 11, 2018
@KrzysztofCwalina KrzysztofCwalina deleted the RemoveOldRob branch January 11, 2018 17:46
@dotnet dotnet deleted a comment from juliusfriedman Jan 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants