Copyright paperwork is done, is there anymore feedback on this? IMO the
main things are as follows:
1) correctness
2) consistency with coding standard
3) solves a problem (new approach reads 8 bytes at a time so we need
something that will break if the code does this wrong e.g. alignment,
endianness)
Cheers
Sam
On Tue, Oct 15, 2024, 10:44 Simon Josefsson <si...@josefsson.org> wrote:
> Thanks this looks better. Let's keep it around for more feedback/review
> until your copyright papers arrive.
>
> /Simon
>
> Sam Russell <sam.h.russ...@gmail.com> writes:
>
> >> Or repeat "Gnulib crc test string" many times.
> >
> > done
> >
> >> This isn't correct until the new implementation is added
> >
> > done
> >
> >> I would make it test much larger
> >> mis-alignment too, how about a ~700 byte string and test up to ~300 byte
> >> mis-alignment?
> >
> > I'm not sure what problem this solves. I've used a >128 byte test string
> > because I have the PCLMUL implementation in mind, but ultimately we're
> > testing mis-alignment on an 8-byte word (slice-by-8 reading into a 64-bit
> > register) or a 16-byte word (PCLMUL reading into a 128-bit register), so
> we
> > could get away with a single off-by-1 test but here we are testing
> > everything from 0-15 bytes of misalignment; a 49-byte misalignment should
> > create the same problem as a 1-byte misalignment.
> >
> >
> > On Tue, 15 Oct 2024 at 09:57, Simon Josefsson <si...@josefsson.org>
> wrote:
> >
> >> Sam Russell <sam.h.russ...@gmail.com> writes:
> >>
> >> > + char plaintext[] = "This file is free software: you can redistribu"
> >> > + "te it and/or modify it under the terms of the "
> >> > + "GNU Lesser General Public License as published"
> >> > + " by th";
> >>
> >> Using license texts like this has at least two problems: 1) license
> >> texts aren't freely licensed so you can't re-use them (although this
> >> snippet is probably too small to be copyrightable), and 2) this may
> >> trigger license review checkers and could result in incorrect license
> >> attribution for this file.
> >>
> >> IMHO it isn't worth this hassle. Use some other freely licensed random
> >> text string instead. Or repeat "Gnulib crc test string" many times.
> >>
> >> > + char data[128 + 16 + 16];
> >>
> >> Use sizeof or some symbol instead of fixed sized like this.
> >>
> >> > + * Test for new CRC32 implementation
> >> > + * The original implementation works on a byte-by-byte basis
> >> > + * but the new one will work on 8 or 16 byte alignments, so
> >> > + * these tests will confirm correct operation with non-aligned
> >> > + * data and data that isn't even multiples of 16 in length.
> >> > + *
> >> > + * The PCLMUL implementation takes 128 bytes at a time on
> >> > + * 16-byte alignment, so we will do 128 + 16 bytes of plaintext
> >> > + * and alter the alignment up to 16 bytes
> >>
> >> This isn't correct until the new implementation is added -- maybe you
> >> could rewrite this into something simpler like "Test that the API handle
> >> differently aligned data."? I would make it test much larger
> >> mis-alignment too, how about a ~700 byte string and test up to ~300 byte
> >> mis-alignment?
> >>
> >> /Simon
> >>
> >> > + */
> >> > +
> >> > + for (i = 0; i < 16; i++)
> >> > + {
> >> > + memcpy(data + i, plaintext, 128 + 16);
> >> > + p = crc32_update_no_xor (0, data + i, 128 + 16);
> >> > + if (p != 0x18c9bfb0)
> >> > + {
> >> > + printf ("aligned c at %lu got %lx\n", i, (unsigned long)
> p);
> >> > + return 1;
> >> > + }
> >> > + }
> >> > +
> >> > +
> >> > return 0;
> >> > }
> >> >
> >>
> > diff --git a/ChangeLog b/ChangeLog
> > index 465b998ccc..d271197d33 100644
> > --- a/ChangeLog
> > +++ b/ChangeLog
> > @@ -1,3 +1,8 @@
> > +2024-10-15 Sam Russell <sam.h.russ...@gmail.com>
> > +
> > + crc: New tests for non-byte-aligned data.
> > + * tests/test-crc.c: New test.
> > +
> > 2024-10-13 Bruno Haible <br...@clisp.org>
> >
> > string-desc: New function string_desc_c_casecmp.
> > diff --git a/tests/test-crc.c b/tests/test-crc.c
> > index 16d2ff08eb..722843de44 100644
> > --- a/tests/test-crc.c
> > +++ b/tests/test-crc.c
> > @@ -21,11 +21,19 @@
> > #include "crc.h"
> >
> > #include <stdio.h>
> > +#include <string.h>
> >
> > int
> > main (int argc, char *argv[])
> > {
> > uint32_t p;
> > + size_t i;
> > + char plaintext[] = "Gnulib crc testsGnulib crc tests"
> > + "Gnulib crc testsGnulib crc tests"
> > + "Gnulib crc testsGnulib crc tests"
> > + "Gnulib crc testsGnulib crc tests"
> > + "Gnulib crc testsGnulib crc tests";
> > + char data[sizeof(plaintext) + sizeof(unsigned long int)];
> >
> > p = crc32_update_no_xor (42, "foo", 3);
> > if (p != 0x46e87f05)
> > @@ -55,5 +63,25 @@ main (int argc, char *argv[])
> > return 1;
> > }
> >
> > + /*
> > + * Test for new CRC32 implementation
> > + * The original implementation works on a byte-by-byte basis
> > + * but new implementations may work on 8 or 16 byte alignments.
> > + * This test will confirm correct operation with non-aligned
> > + * data.
> > + */
> > +
> > + for (i = 0; i < sizeof(unsigned long int); i++)
> > + {
> > + memcpy(data + i, plaintext, sizeof(plaintext));
> > + p = crc32_update_no_xor (0, data + i, sizeof(plaintext));
> > + if (p != 0x654978c3)
> > + {
> > + printf ("aligned c at %lu got %lx\n", i, (unsigned long) p);
> > + return 1;
> > + }
> > + }
> > +
> > +
> > return 0;
> > }
> >
>
