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

Classlib: Add 'rewind' to CollStream #2400

Merged
merged 1 commit into from Jul 3, 2017

Conversation

jamshark70
Copy link
Contributor

I've been using CollStream as part of a parser, and it's very useful to roll back 1 or more characters.

Currently:

stream.pos = stream.pos - 1;

Proposed:

stream.rewind(1);

No rush on this.

(CollStream is as yet undocumented, btw.)

@jamshark70 jamshark70 added the comp: class library SC class library label Oct 14, 2016
@@ -59,6 +59,9 @@ CollStream : IOStream {
pos_ { arg toPos;
pos = toPos.clip(0, collection.size);
}
rewind { |n = 1|
pos = max(0, pos - n);
}
Copy link
Member

Choose a reason for hiding this comment

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

If you provide no argument, stream.rewind, sounds like "fully rewind". Couldn't the default be that?

rewind { |n|
if(n.isNil) { pos = 0 } {
        pos = max(0, pos - n)
}
}

@jamshark70
Copy link
Contributor Author

I see your point. That would mean rewind with no argument would be a synonym for reset -- using it as a synonym may or may not be desirable. Don't know. When I used it as a private extension, I always gave the argument (to be explicit, consistent and readable) so I wouldn't object this change.

@telephon
Copy link
Member

whatever you prefer, either way is fine!

in this way it probably would be:

rewind { |n|
if(n.isNil) { 
       this.reset
 } {
        pos = max(0, pos - n)
}
}

@crucialfelix crucialfelix modified the milestone: 3.9 Nov 4, 2016
@telephon
Copy link
Member

telephon commented Nov 5, 2016

any opinions on that? I'm fine either way.

@telephon
Copy link
Member

I'd like to merge it as it is now, if there are no objections.

@mossheim
Copy link
Contributor

There's an opportunity here to copy the interface from File. If we rework the method as seek, rewind(n) becomes seek(-n, 1), and it also gives us access to end-of-stream offsets seek(0, 2). For some reason the documentation for UnixFILE says this is what pos does, but I don't think that's accurate as the class doesn't actually define that method and its immediate superclass implements it as subclassResponsibility.

@telephon
Copy link
Member

telephon commented Jun 16, 2017

I think, because seek is a little "special", with its numerical behavior switch, we should have both. @brianlheim is it that what you meant?

@mossheim
Copy link
Contributor

I would be OK with both so long as it introduces consistency between File and CollStream etc. This is not so "special" IMO since it is also found in C and Python, just historically motivated. FWIW C/C++ has std::rewind which without an argument returns to the beginning of the stream. That would make more sense to me as a default argument as well. But I'm fine either way, as @jamshark70 pointed out you usually want to move back by one.

@telephon
Copy link
Member

good! (I meant "special" relative to the semantic style elsewhere in sclang, where behaviour is normally not numbered but named, doneAction notwithstanding …)

@mossheim
Copy link
Contributor

Ah, ok :)

@telephon
Copy link
Member

telephon commented Jul 3, 2017

I'll do the seek implementation in a separate PR.

@telephon telephon merged commit db221af into supercollider:master Jul 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: class library SC class library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants