David Barr <davidbarr@xxxxxxxxxx> 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.
> +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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html