# [PATCH] hash: Remove useless init_hash() 7 messages from 2010-07-27 to 2010-07-28. Participants: Stephen Boyd, Jakub Narebski, Ævar Arnfjörð Bjarmason, Linus Torvalds, Jeff King. Thread: https://gitlist.dev/t/24533 ## Stephen Boyd, 2010-07-27 05:36 Subject: [PATCH] hash: Remove useless init_hash() Message-ID: <1280208970-23394-1-git-send-email-bebarino@gmail.com> URL: https://gitlist.dev/e/1280208970-23394-1-git-send-email-bebarino%40gmail.com ``` init_hash() is essentially a memset() so just use that. Signed-off-by: Stephen Boyd --- Documentation/technical/api-hash.txt | 4 ---- diffcore-rename.c | 2 +- hash.h | 7 ------- 3 files changed, 1 insertions(+), 12 deletions(-) diff --git a/Documentation/technical/api-hash.txt b/Documentation/technical/api-hash.txt index e5061e0..7cf64ec 100644 --- a/Documentation/technical/api-hash.txt +++ b/Documentation/technical/api-hash.txt @@ -23,10 +23,6 @@ Data Structures Functions --------- -`init_hash`:: - - Initialize the hash table. - `free_hash`:: Release memory associated with the hash table. diff --git a/diffcore-rename.c b/diffcore-rename.c index df41be5..b355520 100644 --- a/diffcore-rename.c +++ b/diffcore-rename.c @@ -382,7 +382,7 @@ static int find_exact_renames(void) int i; struct hash_table file_table; - init_hash(&file_table); + memset(&file_table, 0, sizeof(file_table)); for (i = 0; i < rename_src_nr; i++) insert_file_table(&file_table, -1, i, rename_src[i].one); diff --git a/hash.h b/hash.h index 69e33a4..418be24 100644 --- a/hash.h +++ b/hash.h @@ -33,11 +33,4 @@ extern void **insert_hash(unsigned int hash, void *ptr, struct hash_table *table extern int for_each_hash(const struct hash_table *table, int (*fn)(void *)); extern void free_hash(struct hash_table *table); -static inline void init_hash(struct hash_table *table) -{ - table->size = 0; - table->nr = 0; - table->array = NULL; -} - #endif -- 1.7.2.19.g9a302 ``` ## Jakub Narebski, 2010-07-27 10:30 Subject: Re: [PATCH] hash: Remove useless init_hash() Message-ID: URL: https://gitlist.dev/e/m3fwz5kykn.fsf%40localhost.localdomain In-Reply-To: <1280208970-23394-1-git-send-email-bebarino@gmail.com> ``` Stephen Boyd writes: > init_hash() is essentially a memset() so just use that. > > Signed-off-by: Stephen Boyd [...] Encapsulation is good. > diff --git a/diffcore-rename.c b/diffcore-rename.c > index df41be5..b355520 100644 > --- a/diffcore-rename.c > +++ b/diffcore-rename.c > @@ -382,7 +382,7 @@ static int find_exact_renames(void) > int i; > struct hash_table file_table; > > - init_hash(&file_table); > + memset(&file_table, 0, sizeof(file_table)); This is IMHO slightly less readable, and doesn't protect against changes in implementation. > for (i = 0; i < rename_src_nr; i++) > insert_file_table(&file_table, -1, i, rename_src[i].one); > > diff --git a/hash.h b/hash.h > index 69e33a4..418be24 100644 > --- a/hash.h > +++ b/hash.h > @@ -33,11 +33,4 @@ extern void **insert_hash(unsigned int hash, void *ptr, struct hash_table *table > extern int for_each_hash(const struct hash_table *table, int (*fn)(void *)); > extern void free_hash(struct hash_table *table); > > -static inline void init_hash(struct hash_table *table) > -{ > - table->size = 0; > - table->nr = 0; > - table->array = NULL; > -} *This* could be replaced by memset. > - > #endif > -- > 1.7.2.19.g9a302 > -- Jakub Narebski Poland ShadeHawk on #git ``` ## Ævar Arnfjörð Bjarmason, 2010-07-27 10:36 Subject: Re: [PATCH] hash: Remove useless init_hash() Message-ID: URL: https://gitlist.dev/e/AANLkTimrKzDOlJzGpzaHWF24lwfr67bmh8RrKj6uMYWB%40mail.gmail.com In-Reply-To: ``` On Tue, Jul 27, 2010 at 10:30, Jakub Narebski wrote: CC-ing Linus since he wrote it (per Documentation/SubmittingPatches). > Stephen Boyd writes: > >> init_hash() is essentially a memset() so just use that. >> >> Signed-off-by: Stephen Boyd > [...] > > Encapsulation is good. > > [..] > > This is IMHO slightly less readable, and doesn't protect against > changes in implementation. Agreed. >>       for (i = 0; i < rename_src_nr; i++) >>               insert_file_table(&file_table, -1, i, rename_src[i].one); >> >> diff --git a/hash.h b/hash.h >> index 69e33a4..418be24 100644 >> --- a/hash.h >> +++ b/hash.h >> @@ -33,11 +33,4 @@ extern void **insert_hash(unsigned int hash, void *ptr, struct hash_table *table >>  extern int for_each_hash(const struct hash_table *table, int (*fn)(void *)); >>  extern void free_hash(struct hash_table *table); >> >> -static inline void init_hash(struct hash_table *table) >> -{ >> -     table->size = 0; >> -     table->nr = 0; >> -     table->array = NULL; >> -} > > *This* could be replaced by memset. No it couldn't? The second argument to memset is just an int, so setting the memory area to 0 isn't portable to systems where the representation of NULL isn't "0". (It's early so I may be misremembering my C..) ``` ## Linus Torvalds, 2010-07-27 15:55 Subject: Re: [PATCH] hash: Remove useless init_hash() Message-ID: URL: https://gitlist.dev/e/AANLkTikjDCookAhrku%3DkGZsKw%2BmoqVXu5517bO3n1pSp%40mail.gmail.com In-Reply-To: <1280208970-23394-1-git-send-email-bebarino@gmail.com> ``` On Mon, Jul 26, 2010 at 10:36 PM, Stephen Boyd wrote: > > init_hash() is essentially a memset() so just use that. I disagree. Yes, technically right now the initialization just zeroes out all the fields, and memset() does the same. But there is no advantage to using memset(), since init_hash() will create the same or better code, and using init_hash() is way more readable. Also, it's not at all the case that init_hash() is always going to be a memset(). Imagine a threaded hash-table with a lock associated with it or something. Now, admittedly that's not necessarily something we'd ever do in git, but I still think it's simply a good idea to have a clear "initialize this" routine, rather than depending on the fact that zeroing it out is sufficient. Linus ``` ## Jeff King, 2010-07-27 19:49 Subject: Re: [PATCH] hash: Remove useless init_hash() Message-ID: <20100727194946.GA11682@coredump.intra.peff.net> URL: https://gitlist.dev/e/20100727194946.GA11682%40coredump.intra.peff.net In-Reply-To: ``` On Tue, Jul 27, 2010 at 10:36:09AM +0000, Ævar Arnfjörð Bjarmason wrote: > >> -static inline void init_hash(struct hash_table *table) > >> -{ > >> -     table->size = 0; > >> -     table->nr = 0; > >> -     table->array = NULL; > >> -} > > > > *This* could be replaced by memset. > > No it couldn't? The second argument to memset is just an int, so > setting the memory area to 0 isn't portable to systems where the > representation of NULL isn't "0". > > (It's early so I may be misremembering my C..) You're remembering your C correctly. It isn't portable, but it is so unlikely on modern machines that we simply don't care (and you will see memsets zero-ing pointers like this all through the git code, so this is certainly not introducing anything new). That being said, I agree with the comments that removing init_hash actually makes the code _less_ readable. You could just replace these three lines with a memset, but why? It's just code churn. -Peff ``` ## Ævar Arnfjörð Bjarmason, 2010-07-27 19:58 Subject: Re: [PATCH] hash: Remove useless init_hash() Message-ID: URL: https://gitlist.dev/e/AANLkTikHLZlBQW8DSZrxeA-RDOh2Iy6hc0p821tDQoN8%40mail.gmail.com In-Reply-To: <20100727194946.GA11682@coredump.intra.peff.net> ``` On Tue, Jul 27, 2010 at 19:49, Jeff King wrote: > On Tue, Jul 27, 2010 at 10:36:09AM +0000, Ævar Arnfjörð Bjarmason wrote: > >> >> -static inline void init_hash(struct hash_table *table) >> >> -{ >> >> -     table->size = 0; >> >> -     table->nr = 0; >> >> -     table->array = NULL; >> >> -} >> > >> > *This* could be replaced by memset. >> >> No it couldn't? The second argument to memset is just an int, so >> setting the memory area to 0 isn't portable to systems where the >> representation of NULL isn't "0". >> >> (It's early so I may be misremembering my C..) > > You're remembering your C correctly. It isn't portable, but it is so > unlikely on modern machines that we simply don't care (and you will see > memsets zero-ing pointers like this all through the git code, so this is > certainly not introducing anything new). Thanks for the confirmation. I was aware that NULL != 0 only occured on long-dead architechtures, but I suspected that some compiler out there would whine if it could statically determine that you were reading in a memzero'd area and using it as NULL. Evidently not, or at least nobody's complained. > That being said, I agree with the comments that removing init_hash > actually makes the code _less_ readable. You could just replace these > three lines with a memset, but why? It's just code churn. Yeah, and for the record it also missed this part in hash.c: void free_hash(struct hash_table *table) { free(table->array); table->array = NULL; table->size = 0; table->nr = 0; } Have fun everyone :) ``` ## Stephen Boyd, 2010-07-28 07:33 Subject: Re: [PATCH] hash: Remove useless init_hash() Message-ID: <4C4FDD63.9050303@gmail.com> URL: https://gitlist.dev/e/4C4FDD63.9050303%40gmail.com In-Reply-To: ``` On 07/27/2010 12:58 PM, Ævar Arnfjörð Bjarmason wrote: > >> That being said, I agree with the comments that removing init_hash >> actually makes the code _less_ readable. You could just replace these >> three lines with a memset, but why? It's just code churn. > Yeah, and for the record it also missed this part in hash.c: > > void free_hash(struct hash_table *table) > { > free(table->array); > table->array = NULL; > table->size = 0; > table->nr = 0; > } > > Have fun everyone :) Ok, seems like nobody thinks this is a good idea so I'm fine with dropping it. The minimal savings (if at all) doesn't seem worth the code churn. One less patch in the queue ;-) ```