Skip to content
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

move crc32c interface into stdlib #24235

Closed
StefanKarpinski opened this issue Oct 20, 2017 · 12 comments
Closed

move crc32c interface into stdlib #24235

StefanKarpinski opened this issue Oct 20, 2017 · 12 comments
Labels
excision Removal of code from Base or the repository
Milestone

Comments

@StefanKarpinski
Copy link
Member

Ref: #21154. Even if we use this internally, it doesn't really belong in Base exports. I'd propose that we have a minimal CRC32c package in the standard library which exports this one function, leaving the definition in base (for internal usage).

@StefanKarpinski StefanKarpinski added excision Removal of code from Base or the repository triage This should be discussed on a triage call labels Oct 20, 2017
@stevengj
Copy link
Member

stevengj commented Oct 21, 2017

What is the advantage of exporting CRC32c vs exporting crc32c?

Oh, I see what you mean. Base wouldn't export anything. The CRC32c module in stdlib would just import Base.crc32c and then export it.

@StefanKarpinski
Copy link
Member Author

Do we actually use this in Base anywhere?

@StefanKarpinski StefanKarpinski added this to the 1.0 milestone Oct 26, 2017
@StefanKarpinski StefanKarpinski removed the triage This should be discussed on a triage call label Oct 26, 2017
@stevengj
Copy link
Member

@StefanKarpinski, yes, it is used in loading.jl for validating cache files.

@JeffBezanson
Copy link
Member

JeffBezanson commented Oct 27, 2017

Actually I'm not sure it's worth making an stdlib package for this. The whole thing is <50 lines of code; there's almost more doc string than code.

@StefanKarpinski
Copy link
Member Author

Then should we just unexport it and only use it internally?

@stevengj
Copy link
Member

Checksumming is a pretty generally useful thing to do; it would be a shame to keep it to ourselves as a private undocumented function.

@stevengj
Copy link
Member

stevengj commented Oct 27, 2017

A trivial stdlib package (that just contains the exports, not any actual code) has the advantage of making this function available, while not tying Julia itself to the API. (e.g. if we someday switched to a different checksum internally, then the code for CRC-32c could be moved entirely into the package without breaking anything.)

@StefanKarpinski
Copy link
Member Author

I don't disagree that it's useful, but it makes much more sense to have larger packages that provide various related checksums with consistent APIs. In fact, there already are multiple such packages: https://github.com/andrewcooke/CRC.jl, https://github.com/fhs/CRC32.jl. The very minimum thing we could do is unexport it and document it as something that may change in the future in 1.x.

@JeffBezanson
Copy link
Member

Yeah that's fair, I guess a trivial wrapper package for the sake of removing the export makes sense.

@stevengj
Copy link
Member

(None of the other checksums is hardware accelerated. In consequence, there is a pretty good argument that this is the only 32-bit checksum you should ever use if you have a choice. This is also why the Base/stdlib function is valuable to expose/document: no other Julia checksum library comes even close to its performance.)

@yuyichao
Copy link
Contributor

None of the other checksums is hardware accelerated

FWIW, ARM/AArch64 have both crc32 and crc32c.

@yuyichao
Copy link
Contributor

Also, the crc32(c) code will likely be the first complex candidate for testing/prototyping runtime cpu feature dispatch API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
excision Removal of code from Base or the repository
Projects
None yet
Development

No branches or pull requests

4 participants