{"thread":{"id":"24533","subject":"[PATCH] hash: Remove useless init_hash()","startedAt":"2010-07-27T05:36:10Z","lastAt":"2010-07-28T07:33:55Z","messageCount":7,"participants":["Stephen Boyd","Jakub Narebski","Ævar Arnfjörð Bjarmason","Linus Torvalds","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"146447","messageId":"1280208970-23394-1-git-send-email-bebarino@gmail.com","threadId":"24533","inReplyTo":null,"subject":"[PATCH] hash: Remove useless init_hash()","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2010-07-27T05:36:10Z","receivedAt":"2010-07-27T05:36:10Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"init_hash() is essentially a memset() so just use that.\n\nSigned-off-by: Stephen Boyd \n---\n Documentation/technical/api-hash.txt | 4 ----\n diffcore-rename.c | 2 +-\n hash.h | 7 -------\n 3 files changed, 1 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/technical/api-hash.txt b/Documentation/technical/api-hash.txt\nindex e5061e0..7cf64ec 100644\n--- a/Documentation/technical/api-hash.txt\n+++ b/Documentation/technical/api-hash.txt\n@@ -23,10 +23,6 @@ Data Structures\n Functions\n ---------\n \n-`init_hash`::\n-\n-\tInitialize the hash table.\n-\n `free_hash`::\n \n \tRelease memory associated with the hash table.\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex df41be5..b355520 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -382,7 +382,7 @@ static int find_exact_renames(void)\n \tint i;\n \tstruct hash_table file_table;\n \n-\tinit_hash(&file_table);\n+\tmemset(&file_table, 0, sizeof(file_table));\n \tfor (i = 0; i < rename_src_nr; i++)\n \t\tinsert_file_table(&file_table, -1, i, rename_src[i].one);\n \ndiff --git a/hash.h b/hash.h\nindex 69e33a4..418be24 100644\n--- a/hash.h\n+++ b/hash.h\n@@ -33,11 +33,4 @@ extern void **insert_hash(unsigned int hash, void *ptr, struct hash_table *table\n extern int for_each_hash(const struct hash_table *table, int (*fn)(void *));\n extern void free_hash(struct hash_table *table);\n \n-static inline void init_hash(struct hash_table *table)\n-{\n-\ttable->size = 0;\n-\ttable->nr = 0;\n-\ttable->array = NULL;\n-}\n-\n #endif\n-- \n1.7.2.19.g9a302\n"},{"id":"146466","messageId":"m3fwz5kykn.fsf@localhost.localdomain","threadId":"24533","inReplyTo":"1280208970-23394-1-git-send-email-bebarino@gmail.com","subject":"Re: [PATCH] hash: Remove useless init_hash()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-07-27T10:30:00Z","receivedAt":"2010-07-27T10:30:00Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Stephen Boyd writes:\n\n> init_hash() is essentially a memset() so just use that.\n> \n> Signed-off-by: Stephen Boyd \n[...]\n\nEncapsulation is good.\n\n> diff --git a/diffcore-rename.c b/diffcore-rename.c\n> index df41be5..b355520 100644\n> --- a/diffcore-rename.c\n> +++ b/diffcore-rename.c\n> @@ -382,7 +382,7 @@ static int find_exact_renames(void)\n> \tint i;\n> \tstruct hash_table file_table;\n> \n> -\tinit_hash(&file_table);\n> +\tmemset(&file_table, 0, sizeof(file_table));\n\nThis is IMHO slightly less readable, and doesn't protect against\nchanges in implementation.\n\n> \tfor (i = 0; i < rename_src_nr; i++)\n> \t\tinsert_file_table(&file_table, -1, i, rename_src[i].one);\n> \n> diff --git a/hash.h b/hash.h\n> index 69e33a4..418be24 100644\n> --- a/hash.h\n> +++ b/hash.h\n> @@ -33,11 +33,4 @@ extern void **insert_hash(unsigned int hash, void *ptr, struct hash_table *table\n> extern int for_each_hash(const struct hash_table *table, int (*fn)(void *));\n> extern void free_hash(struct hash_table *table);\n> \n> -static inline void init_hash(struct hash_table *table)\n> -{\n> -\ttable->size = 0;\n> -\ttable->nr = 0;\n> -\ttable->array = NULL;\n> -}\n\n*This* could be replaced by memset.\n\n> -\n> #endif\n> -- \n> 1.7.2.19.g9a302\n> \n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"146467","messageId":"AANLkTimrKzDOlJzGpzaHWF24lwfr67bmh8RrKj6uMYWB@mail.gmail.com","threadId":"24533","inReplyTo":"m3fwz5kykn.fsf@localhost.localdomain","subject":"Re: [PATCH] hash: Remove useless init_hash()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-27T10:36:09Z","receivedAt":"2010-07-27T10:36:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, Jul 27, 2010 at 10:30, Jakub Narebski wrote:\n\nCC-ing Linus since he wrote it (per Documentation/SubmittingPatches).\n\n> Stephen Boyd writes:\n>\n>> init_hash() is essentially a memset() so just use that.\n>>\n>> Signed-off-by: Stephen Boyd \n> [...]\n>\n> Encapsulation is good.\n>\n> [..]\n>\n> This is IMHO slightly less readable, and doesn't protect against\n> changes in implementation.\n\nAgreed.\n\n>>       for (i = 0; i < rename_src_nr; i++)\n>>               insert_file_table(&file_table, -1, i, rename_src[i].one);\n>>\n>> diff --git a/hash.h b/hash.h\n>> index 69e33a4..418be24 100644\n>> --- a/hash.h\n>> +++ b/hash.h\n>> @@ -33,11 +33,4 @@ extern void **insert_hash(unsigned int hash, void *ptr, struct hash_table *table\n>>  extern int for_each_hash(const struct hash_table *table, int (*fn)(void *));\n>>  extern void free_hash(struct hash_table *table);\n>>\n>> -static inline void init_hash(struct hash_table *table)\n>> -{\n>> -     table->size = 0;\n>> -     table->nr = 0;\n>> -     table->array = NULL;\n>> -}\n>\n> *This* could be replaced by memset.\n\nNo it couldn't? The second argument to memset is just an int, so\nsetting the memory area to 0 isn't portable to systems where the\nrepresentation of NULL isn't \"0\".\n\n(It's early so I may be misremembering my C..)\n"},{"id":"146489","messageId":"AANLkTikjDCookAhrku=kGZsKw+moqVXu5517bO3n1pSp@mail.gmail.com","threadId":"24533","inReplyTo":"1280208970-23394-1-git-send-email-bebarino@gmail.com","subject":"Re: [PATCH] hash: Remove useless init_hash()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-07-27T15:55:46Z","receivedAt":"2010-07-27T15:55:46Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Mon, Jul 26, 2010 at 10:36 PM, Stephen Boyd wrote:\n>\n> init_hash() is essentially a memset() so just use that.\n\nI disagree.\n\nYes, technically right now the initialization just zeroes out all the\nfields, and memset() does the same. But there is no advantage to using\nmemset(), since init_hash() will create the same or better code, and\nusing init_hash() is way more readable.\n\nAlso, it's not at all the case that init_hash() is always going to be\na memset(). Imagine a threaded hash-table with a lock associated with\nit or something. Now, admittedly that's not necessarily something we'd\never do in git, but I still think it's simply a good idea to have a\nclear \"initialize this\" routine, rather than depending on the fact\nthat zeroing it out is sufficient.\n\n Linus\n"},{"id":"146514","messageId":"20100727194946.GA11682@coredump.intra.peff.net","threadId":"24533","inReplyTo":"AANLkTimrKzDOlJzGpzaHWF24lwfr67bmh8RrKj6uMYWB@mail.gmail.com","subject":"Re: [PATCH] hash: Remove useless init_hash()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-07-27T19:49:46Z","receivedAt":"2010-07-27T19:49:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 27, 2010 at 10:36:09AM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> >> -static inline void init_hash(struct hash_table *table)\n> >> -{\n> >> -     table->size = 0;\n> >> -     table->nr = 0;\n> >> -     table->array = NULL;\n> >> -}\n> >\n> > *This* could be replaced by memset.\n> \n> No it couldn't? The second argument to memset is just an int, so\n> setting the memory area to 0 isn't portable to systems where the\n> representation of NULL isn't \"0\".\n> \n> (It's early so I may be misremembering my C..)\n\nYou're remembering your C correctly. It isn't portable, but it is so\nunlikely on modern machines that we simply don't care (and you will see\nmemsets zero-ing pointers like this all through the git code, so this is\ncertainly not introducing anything new).\n\nThat being said, I agree with the comments that removing init_hash\nactually makes the code _less_ readable. You could just replace these\nthree lines with a memset, but why? It's just code churn.\n\n-Peff\n"},{"id":"146517","messageId":"AANLkTikHLZlBQW8DSZrxeA-RDOh2Iy6hc0p821tDQoN8@mail.gmail.com","threadId":"24533","inReplyTo":"20100727194946.GA11682@coredump.intra.peff.net","subject":"Re: [PATCH] hash: Remove useless init_hash()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-27T19:58:39Z","receivedAt":"2010-07-27T19:58:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, Jul 27, 2010 at 19:49, Jeff King wrote:\n> On Tue, Jul 27, 2010 at 10:36:09AM +0000, Ævar Arnfjörð Bjarmason wrote:\n>\n>> >> -static inline void init_hash(struct hash_table *table)\n>> >> -{\n>> >> -     table->size = 0;\n>> >> -     table->nr = 0;\n>> >> -     table->array = NULL;\n>> >> -}\n>> >\n>> > *This* could be replaced by memset.\n>>\n>> No it couldn't? The second argument to memset is just an int, so\n>> setting the memory area to 0 isn't portable to systems where the\n>> representation of NULL isn't \"0\".\n>>\n>> (It's early so I may be misremembering my C..)\n>\n> You're remembering your C correctly. It isn't portable, but it is so\n> unlikely on modern machines that we simply don't care (and you will see\n> memsets zero-ing pointers like this all through the git code, so this is\n> certainly not introducing anything new).\n\nThanks for the confirmation. I was aware that NULL != 0 only occured\non long-dead architechtures, but I suspected that some compiler out\nthere would whine if it could statically determine that you were\nreading in a memzero'd area and using it as NULL. Evidently not, or at\nleast nobody's complained.\n\n> That being said, I agree with the comments that removing init_hash\n> actually makes the code _less_ readable. You could just replace these\n> three lines with a memset, but why? It's just code churn.\n\nYeah, and for the record it also missed this part in hash.c:\n\n void free_hash(struct hash_table *table)\n {\n \tfree(table->array);\n \ttable->array = NULL;\n \ttable->size = 0;\n \ttable->nr = 0;\n }\n\nHave fun everyone :)\n"},{"id":"146583","messageId":"4C4FDD63.9050303@gmail.com","threadId":"24533","inReplyTo":"AANLkTikHLZlBQW8DSZrxeA-RDOh2Iy6hc0p821tDQoN8@mail.gmail.com","subject":"Re: [PATCH] hash: Remove useless init_hash()","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2010-07-28T07:33:55Z","receivedAt":"2010-07-28T07:33:55Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":" On 07/27/2010 12:58 PM, Ævar Arnfjörð Bjarmason wrote:\n>\n>> That being said, I agree with the comments that removing init_hash\n>> actually makes the code _less_ readable. You could just replace these\n>> three lines with a memset, but why? It's just code churn.\n> Yeah, and for the record it also missed this part in hash.c:\n>\n> void free_hash(struct hash_table *table)\n> {\n> \tfree(table->array);\n> \ttable->array = NULL;\n> \ttable->size = 0;\n> \ttable->nr = 0;\n> }\n>\n> Have fun everyone :)\n\nOk, 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 ;-)\n"}]}