Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

unvis_go: leave unicode unchanged with Unvis() #119

Merged
merged 1 commit into from
Feb 10, 2017
Merged

unvis_go: leave unicode unchanged with Unvis() #119

merged 1 commit into from
Feb 10, 2017

Conversation

cyphar
Copy link
Contributor

@cyphar cyphar commented Feb 10, 2017

Because the original code for vis() was ported to Go using the []byte{}
notion, this causes issues for multi-rune bytes (which were not
correctly treated -- and caused loss of information).

Fix this by dealing with []rune instead, which better conveys the
concept at hand. In addition, add tests to ensure that this does not
happen again.

Though, we really should move this code into a library which has a
better test suite -- and the parser itself should be reimplemented to be
less ... 80s.

Fixes: #118
Signed-off-by: Aleksa Sarai asarai@suse.de

Because the original code for vis() was ported to Go using the []byte{}
notion, this causes issues for multi-rune bytes (which were not
correctly treated -- and caused loss of information).

Fix this by dealing with []rune instead, which better conveys the
concept at hand. In addition, add tests to ensure that this does not
happen again.

Though, we _really_ should move this code into a library which has a
better test suite -- and the parser itself should be reimplemented to be
less ... 80s.

Fixes: #118
Signed-off-by: Aleksa Sarai <asarai@suse.de>
@vbatts
Copy link
Owner

vbatts commented Feb 10, 2017

I went back and forth on this when porting that code. I'll test this in a bit.

@cyphar
Copy link
Contributor Author

cyphar commented Feb 10, 2017

I'm going to be honest, I really just want to rewrite the whole thing and drop the old BSD code entirely. I don't like the fact that the old code is written in such an 80s way -- it's impossible to read.

@vbatts
Copy link
Owner

vbatts commented Feb 10, 2017

hahaha. I'm game. I started to, but then wanted it to be cross-compiling sooner, and to ensure it was bug-for-bug compatible went with just a translation.

I'll help if we can tackle it incrementally. Either way.

@vbatts
Copy link
Owner

vbatts commented Feb 10, 2017

Should I merge this for now? and then the overhaul can begin?

@cyphar
Copy link
Contributor Author

cyphar commented Feb 10, 2017

I'd prefer if we merge this now, and then work on the rewrite later.

@vbatts
Copy link
Owner

vbatts commented Feb 10, 2017

LGTM

@vbatts vbatts merged commit 5685419 into vbatts:master Feb 10, 2017
@cyphar cyphar deleted the unvis-utf8 branch February 10, 2017 15:22
@cyphar
Copy link
Contributor Author

cyphar commented Feb 10, 2017

❤️

@vbatts
Copy link
Owner

vbatts commented Feb 10, 2017 via email

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants