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

Projects
None yet
3 participants
@josedonizetti
Member

josedonizetti commented Apr 30, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Apr 30, 2013

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@josedonizetti

josedonizetti Apr 30, 2013

Member

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.

Member

josedonizetti commented Apr 30, 2013

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

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Apr 30, 2013

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@josedonizetti

josedonizetti Apr 30, 2013

Member

Done. Thanks for the tips.

Member

josedonizetti commented Apr 30, 2013

Done. Thanks for the tips.

headius added a commit that referenced this pull request Apr 30, 2013

Merge pull request #682 from josedonizetti/fixing_corrupt_char_problem
Verifying if character is corrupted, meaning that one more char is needed.

@headius headius merged commit 3ad42f9 into jruby:master Apr 30, 2013

1 check was pending

default The Travis build is in progress
Details
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 30, 2013

Member

Thanks for the extra legwork on this one!

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