Skip to content

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 [email protected]
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