Skip to content

[*] changed algorithm for hashing to native redis crc16 CCIT Xmodem#10

Open
h0x91b wants to merge 1 commit intomasterfrom
hash-slots-utf8
Open

[*] changed algorithm for hashing to native redis crc16 CCIT Xmodem#10
h0x91b wants to merge 1 commit intomasterfrom
hash-slots-utf8

Conversation

@h0x91b
Copy link
Collaborator

@h0x91b h0x91b commented Jan 22, 2015

No description provided.

@joaojeronimo
Copy link
Owner

Not cool, doing the V8 - Native ... Native - V8 rountrips makes computing the crc16 significantly slower than computing it in JS, bringing native code in this module will make it impossible to use in environments where people do not have build tools (which is a lot), and if the crc16 algorithm has a problem there must be a fix we can apply to it, replacing it for a native C crc16 doesn't seem a good idea.

@h0x91b
Copy link
Collaborator Author

h0x91b commented Jan 22, 2015

There is no solution in JS for utf-8 multibyte crc16...

@h0x91b h0x91b reopened this Jan 22, 2015
@h0x91b
Copy link
Collaborator Author

h0x91b commented Jan 22, 2015

Try this test:

assert.equal(crc.crc16(new Buffer('123456789')).toString(16), '31c3');
assert.equal(crc.crc16(new Buffer('привет')) % 16384, 7365);
assert.equal(crc.crc16(new Buffer('nht.reach.accounts:زووم')) % 16384, 4107);

I checked 5 JS libraries and all checked libs doing it wrong...

check issue #9

@joaojeronimo
Copy link
Owner

I understand that, still don't think it justifies bringing a native module to the dependencies. Options are 1) fix one of the 5 libraries that don't work 2) accept this as a caveat and tell people to use ASCII keys, shouldn't be too hard.

@h0x91b
Copy link
Collaborator Author

h0x91b commented Jan 23, 2015

Ok, I'll try to fix it

 

Arseniy Pavlenko | CTO

M. +972-54-5417509

scrnz.com

On Fri, Jan 23, 2015 at 1:32 PM, João Jerónimo notifications@github.com
wrote:

I understand that, still don't think it justifies bringing a native module to the dependencies. Options are 1) fix one of the 5 libraries that don't work 2) accept this as a caveat and tell people to use ASCII keys, shouldn't be too hard.

Reply to this email directly or view it on GitHub:
#10 (comment)

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.

2 participants