Deprecate crc32 and move it into std.crc32 (with new interface). #585
Conversation
| @@ -14,6 +14,8 @@ | |||
| // CRC-32 calculation | |||
| module crc32; | |||
|
|
|||
| pragma(msg, "The 'crc32' module has been deprecated. Please use 'std.crc32' instead."); | |||
There was a problem hiding this comment.
It should be "scheduled for deprecation" rather than "deprecated." Also, if we're just moving the module, it should be more like std.contracts was before we removed it - that is, it should publicly import std.crc32, and have its documentation (and the pragma) telling users that it has been scheduled for deprecation and that they should use std.crc32. There's no need to have any implementation in crc32 if none of the implementation is changing.
There was a problem hiding this comment.
Well, in my second commit, I actually changed the API of std.crc32 to sanitize it somewhat, so I'm not sure that's really worth the effort. If users switch to std.crc32, they should just use the new API right away. At least that seems like a more sensible approach, if users are switching away from a deprecated module anyway.
Fixed the message and rebased.
|
Also, you're going to have to merge in the latest HEAD for this to be mergeable. |
|
Well, if you changed the API, then that's different. But if it's a straight move, then there's no need for twice the implementation. |
|
See 61a23808161bcd84c585cc46515767b4a576a106 specifically; I made the API less backwards, fixed the naming, applied some code style, and added |
| } | ||
|
|
||
| uint arrayToCRC32(T)(in T[] arr) pure nothrow | ||
| if (is(T == ubyte) || is(T == byte) || is(T == char)) |
There was a problem hiding this comment.
I think that it's ugly (if not outright evil) that this accepts char. It's not a string. But the original did, so I don't know... I'd still be tempted to disallow it though. People can cast if they really want to pass in a char[].
Regardless, I would point out that if you're going to rewrite it like this, why not just make it take a range?
There was a problem hiding this comment.
I can make it take a range. Removing the ability to pass char arrays probably won't cause too much grief to anyone.
|
Added docs (added myself as author since I may as well take maintainership of this). |
|
Now works on forward ranges and doesn't allow |
|
See also pull request 221. Should we perhaps move crc32 into the |
|
Yeah. Good point. std.crypt.crc32 makes more sense than std.crc32 - particularly since it seems likely that pull request #221 (or something very similar) is going to get merged in. |
|
I think any cryptologist would like to have a word with you if you moved a CRC32 implementation into a cryptography package. ;) I do agree that just flat std.crc32 is a bit undesirable, though. |
|
It's a hashing function just like md5 or sha1 are (and they're slated to go in std.crypt). It's just a sucky one. |
|
jmdavis:
It's not a cryptographic hash, though. There were other suggestions for pull#221 as well, such as |
|
Wouldn't Also, interestingly, there's a Digest::CRC32 in CPAN... |
|
I think so. |
|
Sounds good to me, but the other pull request will need to be changed then. I just think that they should go in the same package. I don't care all that much about the name as long as it's reasonable. |
|
Done. Also made std.stream use the new std.hash.crc32. |
|
This now seems to be passing the auto tester, except on FreeBSD_32: http://d.puremagic.com/test-results/pull.ghtml?runid=183981 The failure is completely unrelated though. I suspect the machine might be so stressed it literally couldn't allocate a semaphore... |
|
I believe that that's a recurring, intermitent failure. It certainly has nothing to do with this (especially, since these changes are in Phobos, no druntime). I'd merge this in, but I was hoping for some sort of response on pull# 221 about using std.hash rather than std.crypt before we did that. |
|
Needs to be rebased due to changes to win32.mak. |
|
I've rebased this into a single commit and also rebased on top of current Phobos. Let's see what the auto tester says. |
|
This is now green in the auto tester. Can someone re-review? |
| * Params: | ||
| * range = The range to compute a CRC32 value for. | ||
| */ | ||
| uint rangeToCRC32(R)(in R range) pure nothrow |
There was a problem hiding this comment.
Maybe CRC of Range? I'm not native speaker but 'to' somehow implies nondestructive conversion to me.
There was a problem hiding this comment.
I'm not sure what you mean. How is this destructive?
There was a problem hiding this comment.
It consumes the range if it's an input range rather than a forward range, but that's normal. I believe that even std.conv.to will do that, so I see no problem with the name. "To" implies a conversion, but it doesn't say anything about what happens to the original. Personally, I'd probably have picked something more like genCRC32, but rangeToCRC32 works just fine.
There was a problem hiding this comment.
I try to only abbreviate words/terms when identifiers get overly long (I'm looking at you, largestPartialIntersectionWeighted ;)), and rangeToCRC32 is pretty bearable IMO.
There was a problem hiding this comment.
gen is a very common and rather obvious abbreviation, and I'd personally be against using the word generate, because gen is plenty clear and much shorter, but rangeToCRC32 works just fine. It's just not what I would have picked.
There was a problem hiding this comment.
It's not length but somehow it seems illogical to me. Anyway I agreee with Jonathan.
|
I think there is not much to review, but here you go ;) |
| /** | ||
| * Initializes a CRC32 value. | ||
| */ | ||
| uint initCRC32() pure nothrow |
There was a problem hiding this comment.
I should have noticed this before, but shouldn't this be a property, or even simply an enum? I'd have expected this to be simply
enum initCRC32 = uint.max;
Or even better,
enum crc32Init = uint.max;
(since initCRC32 is more of a function name, whereas crc32Init looks more like the init property). It seems really odd to make this a function when all it does is return uint.max. I assume that that's what the original did, but if we're improving upon it, it seems rather silly to leave it this way.
There was a problem hiding this comment.
Fixed. This was indeed how the original code looked, and I hadn't really thought of removing some of the pointless abstraction.
|
So is this good to go? |
Please fix |
|
Don't forget to push your changes. |
|
It should all be pushed... |
It looks like it is now, but it wasn't when I checked a few minutes ago. |
Deprecate crc32 and move it into std.crc32 (with new interface).
|
Merged. |
|
Sorry to resurrect this discussion, but shoudn't we use some sort of common interface for all hashes? In pull request #221 both the sha-1 and md5 implementations have a *_CTX struct (btw, couldn't we just use MD5Hash as a name, or just MD5? imho MD5_CTX sounds strange), but this crc32 implementation has nothing similar. I think we need a common interface + some template helpers (isHash, hashType, ...) and we should merge convenience functions like mdFile, SHA1File, etc into new templates which work with every hash. Or are different hashes really so different that they need different interfaces? |
|
MD5's situation is being cleaned up somewhat, since as part of the move to std.hash, its names are being made to match Phobos' current naming conventions - though maybe some additional changes are needed to make the names themselves more appropriate rather than simply following the correct casing. And the SHA-1 stuff was specifically made to look like the MD5 stuff, so there was some attempt to make things the same there, but I'm not sure that it was enough to make hashes intertangeable via templates or not, even for those two. Certainly, you bring up a good point that std.hash.crc32 does not follow the same conventions at all, and that's probably something that we should look at doing (and preferably soon, so that it can be done prior to the next release so that we don't have to tell people to change which functions they're using yet again after we sort that out). |
|
Probably we should revert this pull request for now. I could post a message to the newsgroup regarding the basic design of std.hash and some details (does it really make sense to pass ubyte[16] by ref, do we want classes + interfaces or structs + templates, should finish really reset state, ...) and then adapt crc32 md5 and sha-1 to this new interface next week. |
|
I don't see why you would revert this; that seems very drastic. You can simply alter |
|
Yes, but we don't know when 2.060 will be released (it's been more than 2 months since 2.059) and I don't know how long the refactoring will take (I can promise it would definitely be ready for 2.061). So reverting seems to be the best way to make sure we really won't ship this change in a public release. |
|
We'll have plenty of warning prior to a release to revert the changes to crc32 if we need to do that. I see no reason to go through the trouble of reverting them if we don't have to. But we do need to sort out the std.hash situation fairly promptly if we want it done prior to the next release, since it probably won't be all that long before Walter starts on the beta process for it. |
|
We can simply comment out the parts of the makefiles that put the module into a release, if need be. I really think reverting all of this pull request is unnecessary. |
|
OK, so we can leave it as is till the beta proccess is started. I just posted a message to the D newsgroup regarding the design of std.hash. |
First commit deprecates the crc32 module and copies it into std/crc32.d. Second commit updates the API to be slightly more sane and conforming to Phobos standards. I don't know how we should go about deprecating the old module, but for now, I've just added a pragma(msg, ...); in there.