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

Verifying if character is corrupted, meaning that one more char is needed. #682

Merged
merged 1 commit into from Apr 30, 2013

Conversation

josedonizetti
Copy link
Member

@headius and @enebo this patch fixes the problem we discussed on irc today, specs for readlines and foreach pass, and the File.new('a.txt').gets(5) that Charles did also works.
I came to that comparison "<-1" after debugging the MRI code and seeing that it is exactly what they do there. What do you think about it?

@enebo
Copy link
Member

enebo commented Apr 30, 2013

I have not really dug into MRI to verify this is correct but the <-1 comparison seems like it is difficult for a new reader (or me) to understand what that conditional represents. Do they perhaps have this in some encoding or oniguruma method which hides the -1 part and possibly gives a name which represents this better? If we do roll with this exact patch we minimally have to add a comment explaining the why of the -1 check.

@josedonizetti
Copy link
Member Author

Yup, they hide it on the onigurama, but the name they use is also not understandable for me https://github.com/ruby/ruby/blob/trunk/include/ruby/oniguruma.h#L253, the call that relax the limit to get one more bite on foreach/readlines is https://github.com/ruby/ruby/blob/trunk/io.c#L3058, I can use the same name they are using with a private method and comment the goal of doing it.

@enebo
Copy link
Member

enebo commented Apr 30, 2013

Could you just add that as a sensibly-named method (isIncompleteChar maybe?) in StringSupport.java and the put a comment above that method mentioning MRI name (e.g. // mri: ONIGENC_MBCLEN_NEEDMORE_P)? I am cool with landing this after seeing that code, but the explicit -1 check will end up being confusing...I am betting this is not the only place that macro is used either so we might as well put it in a method.

@josedonizetti
Copy link
Member Author

Done. Thanks for the tips.

headius added a commit that referenced this pull request Apr 30, 2013
Verifying if character is corrupted, meaning that one more char is needed.
@headius headius merged commit 3ad42f9 into jruby:master Apr 30, 2013
@headius
Copy link
Member

headius commented Apr 30, 2013

Thanks for the extra legwork on this one!

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.

None yet

3 participants