Skip to content

Use Encoding::BINARY for JOHAB #472

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

Merged
merged 1 commit into from
Aug 7, 2022

Conversation

eregon
Copy link
Contributor

@eregon eregon commented Jul 30, 2022

* A dummy encoding does not understand JOHAB anyway.
* Creating an extra encoding leads to significant complexity and slowdowns
  in Ruby implementations: https://bugs.ruby-lang.org/issues/18949
@conn.set_client_encoding "JOHAB"
val = @conn.exec("SELECT chr(x'3391'::int)").values[0][0]
expect( val.encoding.name ).to eq( "JOHAB" )
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: this test no longer makes sense/can no longer test anything useful since pg does not create encodings dynamically anymore with this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The test is actually to ensure

if ((i) < ENCODING_INLINE_MAX) \
is working correctly. But in practice there's no way to get this condition failing, without the JOHAB encoding. So we can remove the test.

@eregon
Copy link
Contributor Author

eregon commented Aug 6, 2022

@larskanis Could you review this?

@larskanis
Copy link
Collaborator

IMHO the code for JOHAB is exactly how ruby encodings are intended to be used: It tries to find a corresponding encoding in the running ruby VM, just in case a future ruby version has support for it. And if the search fails, then a placeholder encoding is created, so that the encoding information isn't lost.

That's all OK, but the time of new character encodings is long over. No one invents character encodings these days. If someone would care about JOHAB, he would have implemented support for it in ruby core. So this is dead code.

We already had some discussion about the uselessness of JOHAB encoding in the past #28 (comment) where we already removed the creation of aliases to the dummy encoding here. And it was my intention to remove the code entirely in the near future.

@larskanis larskanis merged commit 28b73d2 into ged:master Aug 7, 2022
@eregon eregon deleted the treat_johab_encoding_as_binary branch August 7, 2022 19:10
@eregon
Copy link
Contributor Author

eregon commented Aug 7, 2022

Great, thank you for merging.

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

2 participants