-
Notifications
You must be signed in to change notification settings - Fork 1
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
Spurious use of hex.Encode for hashmaps #30
Comments
The motivation for the use of
|
Fun fact: this works with yaml:
Edit: removed dumb comment. it appears json is mangling the input bytes, whereas yaml does not. |
@daeMOn63 another idea I received through asking, which was to serialize only on I/O as follows:
I wonder if this, or something similar, is what yaml is doing internally. |
The yaml encoder doesn't seems to apply any specific encoding to map keys. It just support utf-16 (for yaml 1.1 - https://yaml.org/spec/1.1/current.html#id868742) or utf-32 (for 1.2 - https://yaml.org/spec/1.2/spec.html#id2771184) that makes it happy with random bytes. Now on above snippet, this would probably be cleaner to update our code like this, at least to prevent inserting bad characters to the map keys and ensure a consistent encoding / decoding. I would still stick to hex encoding instead of b64, just to avoid those issues with all the different b64 implementations |
Yeah I think the MarshalText is best as well. This way we only incur a cost on encode/decode, which is already expensive relatively speaking. |
Currently we're using hex encoding in order to use hashes as keys in a hashmap, e.g.
e4go/client.go
Line 427 in d0f4bc6
However, this isn't actually necessary, as the following code example shows:
Here the hash is directly encoded as a string type, albeit not one you can print easily. In this, no conversion between formats is required.
We probably want to encode these keys for json output, but during normal operation I'm not sure we need it. What do you think @odeke-em ? (I would key a hashmap in C by passing the raw bytes through the hash function for the hashmap).
The text was updated successfully, but these errors were encountered: