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;
> }
>
signature.asc
Description: PGP signature
