Skip to content

New C# CharStreams static factory class#1775

Merged
parrt merged 1 commit intoantlr:masterfrom
bhamiltoncx:csharp-char-streams
Mar 29, 2017
Merged

New C# CharStreams static factory class#1775
parrt merged 1 commit intoantlr:masterfrom
bhamiltoncx:csharp-char-streams

Conversation

@bhamiltoncx
Copy link
Contributor

Based on the discussion at #1771 (comment) , this brings the Java runtime's CharStreams factory API to C#.

I updated BaseCSharpTest to make use of the new API to illustrate how it works.

@parrt
Copy link
Member

parrt commented Mar 20, 2017

@ericvergnaud Can you comment on this new API?

@ericvergnaud
Copy link
Contributor

Overall looks good except FromStream which unlike other APIs should NOT 'ReadToEnd'.
If you're parsing from a socket, or stdin, EOF could be far away, or even unreachable.

@bhamiltoncx
Copy link
Contributor Author

bhamiltoncx commented Mar 21, 2017 via email

@ericvergnaud
Copy link
Contributor

ericvergnaud commented Mar 21, 2017

You don't need to know.
You let the grammar author deal with it.
i.e. with stdin they can decide to stop when encountering 'exit' token
with socket they can detect a pattern to reset

@parrt
Copy link
Member

parrt commented Mar 21, 2017

I've added important comments here

@bhamiltoncx
Copy link
Contributor Author

You don't need to know.
You let the grammar author deal with it.
i.e. with stdin they can decide to stop when encountering 'exit' token
with socket they can detect a pattern to reset

Hmm. Currently, the design of ICharStream doesn't really allow for reading less than the entire input; all known implementations read the entire contents before constructing ICharStream.

Could you give me an example of what you mean?

@parrt
Copy link
Member

parrt commented Mar 29, 2017

@bhamiltoncx are these consistent now with our tweaks to the Java stream API?

@bhamiltoncx
Copy link
Contributor Author

Yes, everything is consistent, although I didn't do any optimization passes yet, so we don't have a version that takes a size.

@parrt
Copy link
Member

parrt commented Mar 29, 2017

Ok, we can probably worry about optimizing the other targets after 4.7.

@parrt parrt added this to the 4.7 milestone Mar 29, 2017
@parrt parrt merged commit 8284613 into antlr:master Mar 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants