Sam Russell <sam.h.russ...@gmail.com> writes: > This is my implementation of the slice-by-8 algorithm for CRC32 generation.
Thanks!
> I've added a flag CRC_ENABLE_SLICE_BY_8, I'd appreciate if someone can give
> me a hint on how to set this up in the makefile config. I get the
> impression that we want this to be on by default or for the major
> architectures, but still allow people to disable it easily at build
> time.
How about adding something like this to modules/crc:
if test $gl_crc_slice_by_8 != no; then
AC_DEFINE([GL_CRC_SLICE_BY_8], [1],
[Define to get faster but larger CRC32 operation.])
fi
I suggest another name for the identifier GL_CRC_SLICE_BY_8: 'GL_'
prefix for gnulib, and drop the ENABLE since that is implied by the
value.
People using the crc module who want to disable the faster/larger CRC32
implementation can add 'gl_crc_slice_by_8=yes' to their configure.ac
before invoking gnulib.
Do we have a better pattern for this?
> Tests are in the previous patch, coverage there revolves around potential
> alignment issues and using longer sample data to make sure we're hitting
> the codepaths for length > 8 and length not evenly divisible by 8.
I still think we should check larger alignment mismatches in the test
case, it doesn't hurt to test wildly larger mis-alignment for surprising
behaviour.
> + * lib/crc-generate-table.c: Generation code for CRC32 lookup tables
...
> +++ b/lib/crc-generate-table.c
You need to add license terms (GPLv3+?) and follow GNU coding style:
https://www.gnu.org/prep/standards/standards.html#Writing-C
I don't think this file has to be part of the crc module, it is never
used as far as I can tell. Having the generator code around is
important, for completeness. Compare the gzip generator code:
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=73814
That code doesn't have clear licensing terms, so I don't think it should
be used, but it is an example of similar code.
> +// taken from RFC 1952
Don't use //, see the C style guide.
If code is copied then licensing terms has to be clear. RFCs have bad
licensing terms, so maybe we cannot use this code at all.
It would be good to add comments in crc.c before the tables explaining
how to generate them.
> +uint32_t
> +crc32_update_no_xor_slice_by_8 (uint32_t crc, const char *buf)
> +{
> + uint64_t local_buf;
> + memcpy(&local_buf, buf, 8);
That memcpy looks weird, does it really do what you want it to do on all
architectures? Would using something from endian.h be cleaner?
> +uint32_t
> +crc32_update_no_xor_slice_by_n (uint32_t crc, const char *buf, size_t
> num_bytes)
> +{
> + uint64_t local_buf;
> + memcpy(&local_buf, buf, num_bytes);
Same here.
> + local_buf = local_buf ^ crc;
> +
> + if (num_bytes >= 4) {
> + crc = 0;
> + }
> + else {
> + crc = crc >> (num_bytes * 8);
> + }
No need for curly braces here.
> + for (size_t i = 0; i < num_bytes; i++) {
I'd move the variable declaration first in this function.
/Simon
signature.asc
Description: PGP signature
