Add crc64 support by deadalnix · Pull Request #5047 · dlang/phobos

deadalnix · 2017-01-18T15:17:07Z

As per title.

deadalnix · 2017-01-18T15:17:46Z

This is not quite ready yet, but as I can't run the tests manually because of https://issues.dlang.org/show_bug.cgi?id=17107 , I have to rely on autotester.

deadalnix · 2017-01-19T12:23:27Z

Alright this is in better shape now. Tests are green? No idea what circle is unhappy about.

JackStouffer · 2017-01-19T12:37:50Z

cc @wilzbach

deadalnix · 2017-01-19T14:27:06Z

Also, among interesting questions, CRC64 has 2 standards: ECMA and ISO. The algorithm is the same, but the polynomial is different ( 0xD800000000000000 for OSI, 0xC96C5795D7870F42 for ECMA ). ECMA is generally considered better, but maybe we'd like to support both ?

deadalnix · 2017-01-21T13:44:10Z

Ok Added support for both CRC64-ECMA and CRC64-ISO, that wasn't significantly harder, and can be leveraged to do CRC32C and other variations as well.

deadalnix · 2017-01-23T15:05:23Z

I have no idea why circle is complaining. That'd be great if someone would look at this is less than a week.

JackStouffer · 2017-02-06T15:47:55Z

There seems to be a dearth of reviews in Phobos right now.

Do you know of anyone who would be qualified to review this code?

deadalnix · 2017-02-06T16:18:01Z

@JackStouffer I asked someone who dug into this CRC thing with me to figure it out. He'll be able to assert the validitity of the algorithm, but is not an experimented D coder, so that'd be great to have someone checks that.

Also, I'd be glad if someone could explain me what circle is complaining about.

JackStouffer · 2017-02-07T05:02:50Z

std/digest/crc.d

+
+/**
+ * Generic Template API used for CRC32 and CRC64 implementations.
+ * See $(D std.digest.digest) for differences between template and OOP API.


needs Params section for N and P

JackStouffer · 2017-02-07T05:04:56Z

std/digest/crc.d

+
+        static immutable T[256][8] tables = genTables!T(P);
+
+        alias R = ubyte[T.sizeof];


This needs to be documented as it shows up in the docs

JackStouffer · 2017-02-08T15:57:32Z

Also, needs a changelog entry

wilzbach · 2017-02-17T10:23:39Z

std/digest/crc.d

        unittest
        {
-            CRC32 dig;
+            CRC dig;


That's the reason why CircleCi fails with:

out/std_digest_crc.d(46): Error: struct std.digest.crc.CRC(uint N, ulong P) if (N == 32 || N == 64) is used as a type out/std_digest_crc.d(59): Error: struct std.digest.crc.CRC(uint N, ulong P) if (N == 32 || N == 64) is used as a type out/std_digest_crc.d(71): Error: struct std.digest.crc.CRC(uint N, ulong P) if (N == 32 || N == 64) is used as a type out/std_digest_crc.d(73): Error: undefined identifier 'R'

The script at CircleCi seperates all public unittests and tries to run them with the newly compiled release library to ensure that all unittests on dlang.org are runnable as they are.

Btw I remember there was a long discussion last summer about the problems of using unittest blocks in templated code - in short in theory it's discouraged at Phobos because the unittest block gets instantiated very every new instance.

Indeed, the test is generated for each templates, it is a 3 line tests, so I don't think there is really any problem here.

deadalnix · 2017-03-03T13:36:44Z

Sadly I find myself unable to continue this work.

https://issues.dlang.org/show_bug.cgi?id=17236

I was already required to jump though hoops to run the tests, but this is getting out of hands.

wilzbach · 2017-03-03T22:56:04Z

Sadly I find myself unable to continue this work.
I was already required to jump though hoops to run the tests, but this is getting out of hands.

As mentioned on the NG I am still not sure what your exact problem is.
In any case, you can always build a module directly if you have a recent DMD compiler, e.g.:

source $(curl https://i.dlang.io | bash -s dmd-nightly -a)

deadalnix · 2017-03-08T14:37:14Z

Added doc for N, P and R.

Rebased. I got various merge conflicts for style changes that arguably do not improve the code in any way. Especially the one which puts template constraint on their own line.

JackStouffer · 2017-03-08T14:49:13Z

std/digest/crc.d

+ *      CRC64-ECMA of data
+ */
+//simple alias doesn't work here, hope this gets inlined...
+ubyte[8] crc64ECMAOf(T...)(T data)


According to Circle, this needs an example

I'm not sure what I'm expected to do here. I looked at crc32Of and there is no example. Is there a piece of code I can look at to know what to do ?

JackStouffer · 2017-03-08T14:49:21Z

std/digest/crc.d

+ *      CRC64-ISO of data
+ */
+//simple alias doesn't work here, hope this gets inlined...
+ubyte[8] crc64ISOOf(T...)(T data)


this too too !

JackStouffer · 2017-03-08T14:50:10Z

@deadalnix Was that person you mentioned able to verify the CRC code correctness?

deadalnix · 2017-03-08T15:00:39Z

@ftrader , we need you :)

deadalnix · 2017-03-08T15:31:03Z

You can check it matches the test vector from go here: https://golang.org/src/hash/crc64/crc64_test.go

JackStouffer · 2017-05-15T20:10:43Z

@deadalnix Sorry, I only just saw your comment about verification.

Any chance you can fix the public example issue so this can be pulled?

CyberShadow · 2017-05-16T06:49:16Z

FWIW, I had a CRC64 (ECMA variant) implementation on top of std.digest lying around. I plugged the unittests from here into my implementation, and everything worked.

deadalnix · 2017-05-21T06:40:52Z

OK @CyberShadow confirm that the result are good, which is pretty convincing the algorithm is correct (this is a hash function after all, so you'd expect to get very different results).

So I rebased and I want to get that in good shape so we can merge it.

deadalnix · 2017-05-21T06:58:17Z

Looks like circle is happy. Do we move forward with that one ?

JackStouffer · 2017-05-21T16:33:31Z

std/digest/crc.d

+         * Type of the finished CRC hash.
+         * ubyte[8] if N is 32, ubyte[16] if N is 64.
+         */
+        alias R = ubyte[T.sizeof];


Shouldn't R be ubyte[4] for 32 bit? Because finish used to return ubyte[4] originally and now it returns ubyte[8].

The code is correct, but the comment is wrong. Fixing.

…ymbol without making circle unhappy.

deadalnix · 2017-05-21T21:04:57Z

Fixed.

deadalnix force-pushed the crc64 branch 8 times, most recently from bcee6d5 to 689e438 Compare January 18, 2017 21:37

JackStouffer added the Severity:Enhancement label Jan 19, 2017

deadalnix force-pushed the crc64 branch from 689e438 to 8f95e44 Compare January 21, 2017 13:43

JackStouffer added the Review:Needs Review label Jan 21, 2017

deadalnix force-pushed the crc64 branch from 44f5f64 to 5b4ebd1 Compare January 23, 2017 14:24

JackStouffer reviewed Feb 7, 2017

View reviewed changes

JackStouffer added this to the 2.074.0 milestone Feb 8, 2017

wilzbach reviewed Feb 17, 2017

View reviewed changes

wilzbach added Merge:Needs Rebase Review:Needs Work labels Feb 21, 2017

JackStouffer removed the Review:Needs Work label Mar 7, 2017

JackStouffer added this to the 2.075.0 milestone Mar 7, 2017

JackStouffer removed this from the 2.074.0 milestone Mar 7, 2017

deadalnix force-pushed the crc64 branch from 5b4ebd1 to 38206ba Compare March 8, 2017 14:35

JackStouffer reviewed Mar 8, 2017

View reviewed changes

JackStouffer removed the Merge:Needs Rebase label Mar 8, 2017

deadalnix force-pushed the crc64 branch 2 times, most recently from 601c8b7 to f216ed6 Compare March 15, 2017 13:12

deadalnix force-pushed the crc64 branch from f216ed6 to 0589f39 Compare May 21, 2017 06:40

JackStouffer reviewed May 21, 2017

View reviewed changes

deadalnix added 5 commits May 21, 2017 23:04

Add crc64 support

d3e00ae

Add ECMA and ISO CRC64 variations

a4d9b57

Make the CRC struct public. It seems like one can't alias a private s…

0ea46ff

…ymbol without making circle unhappy.

Add more doc

6d4be23

Remove unittests which makes circle unhappy.

05a4c06

deadalnix force-pushed the crc64 branch from 0589f39 to 05a4c06 Compare May 21, 2017 21:04

JackStouffer approved these changes May 21, 2017

View reviewed changes

JackStouffer added Merge:auto-merge and removed Review:Needs Review labels May 21, 2017

dlang-bot merged commit 976eb43 into dlang:master May 21, 2017

JackStouffer mentioned this pull request May 22, 2017

Moved ddoc unittest block back to its proper position in std.digest.crc #5414

Merged

wilzbach mentioned this pull request Jul 7, 2017

Update std.digest BOOKTABLE #5570

Merged


		static immutable T[256][8] tables = genTables!T(P);

		alias R = ubyte[T.sizeof];

Uh oh!

Conversation

deadalnix commented Jan 18, 2017

Uh oh!

deadalnix commented Jan 18, 2017

Uh oh!

deadalnix commented Jan 19, 2017

Uh oh!

JackStouffer commented Jan 19, 2017

Uh oh!

deadalnix commented Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deadalnix commented Jan 21, 2017

Uh oh!

deadalnix commented Jan 23, 2017

Uh oh!

JackStouffer commented Feb 6, 2017

Uh oh!

deadalnix commented Feb 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackStouffer commented Feb 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deadalnix commented Mar 3, 2017

Uh oh!

wilzbach commented Mar 3, 2017

Uh oh!

deadalnix commented Mar 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackStouffer commented Mar 8, 2017

Uh oh!

deadalnix commented Mar 8, 2017

Uh oh!

deadalnix commented Mar 8, 2017

Uh oh!

JackStouffer commented May 15, 2017

Uh oh!

CyberShadow commented May 16, 2017

Uh oh!

deadalnix commented May 21, 2017

Uh oh!

deadalnix commented May 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deadalnix commented May 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

deadalnix commented Jan 19, 2017 •

edited

Loading