Re: [RFC/PATCH 1/3] protobuf: minimal implementation for compact in-memory structures
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 23, 2011, 17:22 UTC
- Message-ID
- <7vliws31k8.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1308728011-14136-2-git-send-email-davidbarr@google.com>
David Barr <davidbarr@google.com> writes:
> One struct to capture all types, just 4 methods: decode_message, > encode_message, sizeof_message, hash_field.
Adding to the review from yesterday, hash_field() looked quite out of place. If you are going to implement a hash table that holds protobuf objects in a separate file/module, I would imagine the function belongs there, not here.
Show 20 quoted lines
> +uint32_t hash_field(const struct protobuf_field *field)
> +{
> + uint32_t hc = 0;
> + switch (field->type) {
> + case WT_VARINT:
> + case WT_64BIT:
> + hc = (0x9e3779b97f4a7c15ull * field->val.num) >> 32;
> + break;
> + case WT_SHA1:
> + memcpy(&hc, field->val.bin.ptr, sizeof(hc));
> + break;
> + case WT_STRING:
> + hc = x65599(field->val.bin.ptr, field->val.bin.len);
> + break;
> + case WT_32BIT:
> + hc = 0x9e3779b9ul * (uint32_t)field->val.num;
> + break;
> + }
> + return hc;
> +}It all depends on how you envision a "hash table of protobuf objects" is to be used, but what is the point of using a complex math for 64BIT/32BIT integer values? If you plan to have different kinds of protobuf objects thrown into a single hash table, it may make sense, but without a crystal ball it was kind of hard to judge.