I don't think there is a need to cover any conceivable checksum function. If there are concrete users that need, e.g Adler-32, then sure, we should add it. But I'd rather hold off on custom or generic crc polynomial support.

I would like to have a crc-16 for gtranslator. Any posibility of adding it?

Someone needs to provide patches...

(In reply to comment #2)
> I would like to have a crc-16 for gtranslator. Any posibility of adding it?

Do you need a specific CRC-16 polynomial or just some kind of 16 bit checksum?

Created attachment 125846 [details] [review]
gchecksum-crc.diff

This patch adds a table-driven CRC16 and CRC32 implementation to GChecksum.

Now the question is whether we want a table-driven implementation (increases library size) or if a direct implementation is good enough (slower).

Also the used polynomials could be changed or one could choose different init values or XOR the input/output by some value.

Any comments on this? :)

Comment please?

Review of attachment 125846 [details] [review]:

the patch needs to be updated to git master, at the very least.

::: glib/gchecksum.c
@@ +76,3 @@
+#define CRC16_DIGEST_LEN	2
+typedef struct {
+  guint16 digest;;

double semi-colon.

@@ +1099,3 @@
+crc16_sum_init (Crc16sum *crc16)
+{
+  crc16->digest = 0xffff;

0xffff looks like a handy constant that should probably be put under a #define, given its use as an initializer and a mask.

@@ +1110,3 @@
+  guint16 crc = crc16->digest;
+  
+  while (length--) {

coding style: the curly braces go on a different indentation level.

@@ +1227,3 @@
+  guint32 crc = crc32->digest;
+  
+  while (length--) {

coding style: the curly braces go on a different indentation level.

also, to reiterate what Matthias said in comment 1, I'd like to see some concrete use case for these kinds of checksums, possibly in the core GNOME stack, before committing to having them into GChecksum.

Created attachment 233572 [details] [review]
add crc32 and crc16 to GChecksum v2

I updated the patch based on Emmanuele's review in comment 8. I will add a test in a follow-up patch. In reply to comment 1 and comment 9, I would like to use the CRC-32 support in EasyTAG (not a core GNOME module!), which is used a data integrity check for MP3 data, so that I can drop the internal version:

http://git.gnome.org/browse/easytag/tree/src/crc32.c?h=wip/easytag-next

I do not have a use for CRC-16.

So what should we do about this? Seems that GLib developers don't care to have this in GLib, maybe just WONTFIX it?

(In reply to comment #11)
> So what should we do about this? Seems that GLib developers don't care to have
> this in GLib, maybe just WONTFIX it?

Fine by me, as I have not found time to write a test as of yet.

FWIW, I need CRC-32 for DFU support too.

Created attachment 315028 [details] [review]
Add tests for CRC16 and CRC32 support

Added tests for CRC16 and CRC32.

I did however notice something - the CRC values seem to be inverted. The CRC32 of an empty string is ffffffff - shouldn't this be 00000000?

[Dropping Adler-32 from the summary; that's more-or-less only used by zlib, and there's no real reason to reimplement that based on GLib (and it's not a CRC anyway so it could go in a separate bug if you really did want to).]

Comment on attachment 233572 [details] [review]
add crc32 and crc16 to GChecksum v2

>+ * CRC-16 is the CCITT polynomial: 0x1021
>+ * CRC-32 is the IEEE 802.3 polynomial: 0x04c11db7

So while those do appear to be the most-commonly-used 16-bit and 32-bit CRC polynomials (and, eg, that CRC-32 is the one Richard needs for DFU), it seems a little weird to implement just those ones and call them "CRC-16" and "CRC-32" as though they're the only choices. (Although admittedly, the Wikipedia article gives specific names to every 32-bit CRC *except* that one, which it just calls "CRC-32"...)

So maybe G_CHECKSUM_CRC16_CCITT and G_CHECKSUM_CRC32 (maybe with an alias of G_CHECKSUM_CRC32_IEEE? Or vice versa.). And maybe also implement G_CHECKSUM_CRC32C (the mathematically-proven-to-be-better version that seems to be used in most newer protocols/formats, like SCTP, iSCSI, ext4, and btrfs).

Having large pre-computed tables seems excessive, though at the same time they're not *that* large... OTOH, is this really going to be used in situations where speed is all that important? If not, then I think I'd go with

  GChecksum *g_checksum_new_crc (guint length, guint64 polynomial);
  #define G_CHECKSUM_CRC16_CCITT 0x1021
  #define G_CHECKSUM_CRC32       0x04c11db7

and note in the docs that you can use any 16- or 32-bit polynomial you want, but other lengths are not guaranteed to work (but leaving open the possibility that we might make 12-bit, 20-bit, 64-bit, etc, CRCs work later if there was some use case for them). (And if speed did turn out to be important, we could optimize specific polynomials despite the generic API.)

(In reply to Robert Ancell from comment #14)
> I did however notice something - the CRC values seem to be inverted. The
> CRC32 of an empty string is ffffffff - shouldn't this be 00000000?

Obviously the test values should be generated by something *other* then GChecksum...

If someone wants to update those patches, I’d be happy for CRC-32 support to be included in GLib, since it seems like there are enough users wanting it. CRC-16 is a different question, since only gtranslator was asking for it, and gtranslator seems to be pretty inactive these days.

(Marking patches as needs-work as per comment #18.)

Review of attachment 233572 [details] [review]:

.

Review of attachment 315028 [details] [review]:

.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/133.