On 1/22/25 5:17 PM, Taylor Blau wrote:
On Fri, Dec 20, 2024 at 05:19:48PM +0000, Derrick Stolee via GitGitGadget wrote:
From: Derrick Stolee <stolee@xxxxxxxxx>
+static inline uint32_t pack_name_hash_fn(const char *name)
+{
+ switch (name_hash_version)
+ {
This is definitely a nitpick, but the opening curly brace should appear
on the preceding line. I don't think our CodingGuidelines explicitly say
this. But in my head the convention is that only function bodies have
their enclosing braces on their own line, as opposed to:
You're right, I messed up on the switch statement.
static inline uint32_t pack_name_hash_fn(const char *name) {
but based on my expectations and your earlier comment I think that this
suggestion is a copy/paste error and you meant to highlight the switch
statement.
+ validate_name_hash_version();
+ if (write_bitmap_index && name_hash_version != 1) {
+ warning(_("currently, --write-bitmap-index requires --name-hash-version=1"));
+ name_hash_version = 1;
+ }
+
Hmm. validate_name_hash_version() is its own function, which seems good,
but then we do further validation on it here. I wonder if we should
either move the latter step into validate_name_hash_version(), which
would require us to pass a pointer to name_hash_version, or assign it
the return value of validate_name_hash_version() (assuming it were
changed to return the appropriate type instead of void).
I think that we are probably pretty far into bike-shedding territory,
but figured I'd share as it jumped out to me while reviewing.
Sounds good. I'll try to minimize the uses of name_hash_version outside
of the cluster of methods that implement its details. This may become
more complicated later when the timing of these checks is more interesting.
Thanks,
-Stolee
