{"thread":{"id":"36046","subject":"[PATCH v2] cache_tree_find(): remove redundant checks","startedAt":"2014-03-04T08:31:08Z","lastAt":"2014-03-05T18:40:08Z","messageCount":11,"participants":["Michael Haggerty","David Kastrup","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"235978","messageId":"1393921868-4382-1-git-send-email-mhagger@alum.mit.edu","threadId":"36046","inReplyTo":null,"subject":"[PATCH v2] cache_tree_find(): remove redundant checks","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-04T08:31:08Z","receivedAt":"2014-03-04T08:31:08Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The beginning of the loop ensures that slash can never be NULL.  So\ndon't keep checking whether it is NULL later in the loop.\n\nFurthermore, there is no need for an early\n\n    return it;\n\nfrom the loop if slash points at the end of the string, because that\nis exactly what will happen when the while condition fails at the\nstart of the next iteration.\n\nHelped-by: David Kastrup <dak@gnu.org>\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\nI incorporated David's suggestion, and then realized that yet another\ncheck was superfluous, so I removed that one too.\n\n cache-tree.c | 15 ++++++---------\n 1 file changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 0bbec43..19252c3 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -551,25 +551,22 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat\n \tif (!it)\n \t\treturn NULL;\n \twhile (*path) {\n-\t\tconst char *slash;\n \t\tstruct cache_tree_sub *sub;\n+\t\tconst char *slash = strchr(path, '/');\n \n-\t\tslash = strchr(path, '/');\n \t\tif (!slash)\n \t\t\tslash = path + strlen(path);\n-\t\t/* between path and slash is the name of the\n-\t\t * subtree to look for.\n+\t\t/*\n+\t\t * Between path and slash is the name of the subtree\n+\t\t * to look for.\n \t\t */\n \t\tsub = find_subtree(it, path, slash - path, 0);\n \t\tif (!sub)\n \t\t\treturn NULL;\n \t\tit = sub->cache_tree;\n-\t\tif (slash)\n-\t\t\twhile (*slash && *slash == '/')\n-\t\t\t\tslash++;\n-\t\tif (!slash || !*slash)\n-\t\t\treturn it; /* prefix ended with slashes */\n \t\tpath = slash;\n+\t\twhile (*path == '/')\n+\t\t\tpath++;\n \t}\n \treturn it;\n }\n-- \n1.9.0\n"},{"id":"235984","messageId":"87ppm2i9ot.fsf@fencepost.gnu.org","threadId":"36046","inReplyTo":"1393921868-4382-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2] cache_tree_find(): remove redundant checks","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-03-04T09:40:02Z","receivedAt":"2014-03-04T09:40:02Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> The beginning of the loop ensures that slash can never be NULL.  So\n> don't keep checking whether it is NULL later in the loop.\n>\n> Furthermore, there is no need for an early\n>\n>     return it;\n>\n> from the loop if slash points at the end of the string, because that\n> is exactly what will happen when the while condition fails at the\n> start of the next iteration.\n\nHm.  Another suggestion.  You have\n\n\t\tconst char *slash = strchr(path, '/');\n \t\tif (!slash)\n \t\t\tslash = path + strlen(path);\n[...]\n\t\tsub = find_subtree(it, path, slash - path, 0);\n[...]\n\t\tpath = slash;\n\t\twhile (*path == '/')\n\t\t\tpath++;\n\t}\n\nAt the price of introducing another variable, this could be\n\n\t\tconst char *slash = strchr(path, '/');\n \t\tsize_t len = slash ? slash - path : strlen(path);\n[...]\n\t\tsub = find_subtree(it, path, len, 0);\n[...]\n                if (!slash)\n\t\t\tbreak;\n\t\tfor (path = slash; *path == '/';)\n\t\t\tpath++;\n\t}\n\nThis introduces another variable and another condition.  The advantage\nis that \"slash\" indeed points at a slash or is NULL, so the variable\nnames correspond better to what happens.  Alternatively, it might make\nsense to rename \"slash\" into \"end\" or \"endpart\" or whatever.  Since\nI can't think of a pretty name, I lean towards preferring the latter\nversion as it reads nicer.  I prefer code to read like children's books\nrather than mystery novels.\n\n-- \nDavid Kastrup\n"},{"id":"235985","messageId":"5315A974.5000104@alum.mit.edu","threadId":"36046","inReplyTo":"87ppm2i9ot.fsf@fencepost.gnu.org","subject":"Re: [PATCH v2] cache_tree_find(): remove redundant checks","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-04T10:22:44Z","receivedAt":"2014-03-04T10:22:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/04/2014 10:40 AM, David Kastrup wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> The beginning of the loop ensures that slash can never be NULL.  So\n>> don't keep checking whether it is NULL later in the loop.\n>>\n>> Furthermore, there is no need for an early\n>>\n>>     return it;\n>>\n>> from the loop if slash points at the end of the string, because that\n>> is exactly what will happen when the while condition fails at the\n>> start of the next iteration.\n> \n> Hm.  Another suggestion.  You have\n> \n> \t\tconst char *slash = strchr(path, '/');\n>  \t\tif (!slash)\n>  \t\t\tslash = path + strlen(path);\n> [...]\n> \t\tsub = find_subtree(it, path, slash - path, 0);\n> [...]\n> \t\tpath = slash;\n> \t\twhile (*path == '/')\n> \t\t\tpath++;\n> \t}\n> \n> At the price of introducing another variable, this could be\n> \n> \t\tconst char *slash = strchr(path, '/');\n>  \t\tsize_t len = slash ? slash - path : strlen(path);\n> [...]\n> \t\tsub = find_subtree(it, path, len, 0);\n> [...]\n>                 if (!slash)\n> \t\t\tbreak;\n> \t\tfor (path = slash; *path == '/';)\n> \t\t\tpath++;\n> \t}\n> \n> This introduces another variable and another condition.  The advantage\n> is that \"slash\" indeed points at a slash or is NULL, so the variable\n> names correspond better to what happens.  Alternatively, it might make\n> sense to rename \"slash\" into \"end\" or \"endpart\" or whatever.  Since\n> I can't think of a pretty name, I lean towards preferring the latter\n> version as it reads nicer.  I prefer code to read like children's books\n> rather than mystery novels.\n\nI think we're reaching the point of diminishing returns here.  I can't\nmuster a strong feeling either way about your suggestion to add a \"len\"\nvariable.\n\nBTW, I purposely didn't use a \"for\" loop at the end (even though I\nusually like them) because I wanted to keep it prominent that path is\nbeing updated to the value of slash.  Putting that assignment in a for\nloop makes it easy to overlook because it puts \"path\" in the spot that\nusually holds an inconsequential iteration variable.\n\nYMMV.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"235987","messageId":"87ha7ei75s.fsf@fencepost.gnu.org","threadId":"36046","inReplyTo":"5315A974.5000104@alum.mit.edu","subject":"Re: [PATCH v2] cache_tree_find(): remove redundant checks","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-03-04T10:34:39Z","receivedAt":"2014-03-04T10:34:39Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> BTW, I purposely didn't use a \"for\" loop at the end (even though I\n> usually like them) because I wanted to keep it prominent that path is\n> being updated to the value of slash.  Putting that assignment in a for\n> loop makes it easy to overlook because it puts \"path\" in the spot that\n> usually holds an inconsequential iteration variable.\n\nReasonable.\n\n-- \nDavid Kastrup\n"},{"id":"236040","messageId":"xmqq8uspk72g.fsf@gitster.dls.corp.google.com","threadId":"36046","inReplyTo":"1393921868-4382-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2] cache_tree_find(): remove redundant checks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-04T21:05:59Z","receivedAt":"2014-03-04T21:05:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n>  \twhile (*path) {\n> -\t\tconst char *slash;\n>  \t\tstruct cache_tree_sub *sub;\n> +\t\tconst char *slash = strchr(path, '/');\n>  \n> -\t\tslash = strchr(path, '/');\n>  \t\tif (!slash)\n>  \t\t\tslash = path + strlen(path);\n\nIsn't the above a strchrnul()?\n\nCombining a freestanding decl with intializer assignment to lose one\nline is sort of cheating on the line count, but replacing the three\nlines with a single strchrnul() would be a real code reduction ;-)\n\n> -\t\t/* between path and slash is the name of the\n> -\t\t * subtree to look for.\n> +\t\t/*\n> +\t\t * Between path and slash is the name of the subtree\n> +\t\t * to look for.\n>  \t\t */\n>  \t\tsub = find_subtree(it, path, slash - path, 0);\n>  \t\tif (!sub)\n>  \t\t\treturn NULL;\n>  \t\tit = sub->cache_tree;\n> -\t\tif (slash)\n> -\t\t\twhile (*slash && *slash == '/')\n> -\t\t\t\tslash++;\n> -\t\tif (!slash || !*slash)\n> -\t\t\treturn it; /* prefix ended with slashes */\n>  \t\tpath = slash;\n> +\t\twhile (*path == '/')\n> +\t\t\tpath++;\n>  \t}\n>  \treturn it;\n>  }\n"},{"id":"236042","messageId":"xmqq4n3dk6te.fsf@gitster.dls.corp.google.com","threadId":"36046","inReplyTo":"1393921868-4382-1-git-send-email-mhagger@alum.mit.edu","subject":"[microproject idea]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-04T21:11:25Z","receivedAt":"2014-03-04T21:11:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Find places where we scan a string twice unnecessarily, once with\nstrchr() and then with strlen(), e.g.\n\n\tconst char *colon = strchr(name, ':');\n\tint namelen = colon ? colon - name : strlen(name);\n\nand rewrite such a pattern using strchrnul() as appropriate.\n\nThe above example can become\n\n\tconst char *colon = strchrnul(name, ':');\n        int namelen = colon - name;\n"},{"id":"236048","messageId":"531652B9.4000001@alum.mit.edu","threadId":"36046","inReplyTo":"xmqq8uspk72g.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] cache_tree_find(): remove redundant checks","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-04T22:24:57Z","receivedAt":"2014-03-04T22:24:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/04/2014 10:05 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>>  \twhile (*path) {\n>> -\t\tconst char *slash;\n>>  \t\tstruct cache_tree_sub *sub;\n>> +\t\tconst char *slash = strchr(path, '/');\n>>  \n>> -\t\tslash = strchr(path, '/');\n>>  \t\tif (!slash)\n>>  \t\t\tslash = path + strlen(path);\n> \n> Isn't the above a strchrnul()?\n\nOh, cool, I never realized that this GNU extension was blessed for use\nin Git.  Will change.\n\n> Combining a freestanding decl with intializer assignment to lose one\n> line is sort of cheating on the line count, but replacing the three\n> lines with a single strchrnul() would be a real code reduction ;-)\n\nI suppose you are just teasing me, but for the record I consider line\ncount only a secondary metric.  The reason for combining initialization\nwith declaration is to reduce by one the number of times that the reader\nhas to think about that variable when analyzing the code.\n\nAnd as I am writing this I realize that after converting to the use of\nstrchrnul(), sub can also be initialized at its declaration.\n\nI really wish we could mix declarations with statements because I think\nit is a big help to readability.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"236054","messageId":"xmqqr46himd1.fsf@gitster.dls.corp.google.com","threadId":"36046","inReplyTo":"531652B9.4000001@alum.mit.edu","subject":"Re: [PATCH v2] cache_tree_find(): remove redundant checks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-04T23:18:34Z","receivedAt":"2014-03-04T23:18:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n>> Isn't the above a strchrnul()?\n>\n> Oh, cool, I never realized that this GNU extension was blessed for use\n> in Git.  Will change.\n\nWe do have our own fallbacks for non-glibc platforms, so it should\nbe safe.\n\n>> Combining a freestanding decl with intializer assignment to lose one\n>> line is sort of cheating on the line count, but replacing the three\n>> lines with a single strchrnul() would be a real code reduction ;-)\n>\n> I suppose you are just teasing me, but for the record I consider line\n> count only a secondary metric.  The reason for combining initialization\n> with declaration is to reduce by one the number of times that the reader\n> has to think about that variable when analyzing the code.\n> ...\n> I really wish we could mix declarations with statements because I think\n> it is a big help to readability.\n\nUnfortunately, I think we are in violent disagreement.\n\nA variable declaration block with initializations on only some but\nnot all variables is extremely annoying.  If none of the variable\ndeclaration has initialization (or initialization to trivial values\nthat do not depend on the logic flow), and the first statement is\nseparated from the decl block, then I do not have to read the decl\npart when reading the code/logic *at all* (the compiler will find\nmissing variables, variables declared as a wrong type, etc.).\n\nIn other words, a trivial initialization at the beginning of the\nblock, if the logic flow only sometimes makes assignment to the\nvariable, is perfectly fine, e.g.\n\n\tconst char *string = NULL;\n\n\tif (...) {\n        \tstring = ...\n\t}                \n\nBut I would wish people stop doing this:\n\n\tconst char *string = strchrnul(name, ':');\n\n\t... the real logic of the block that uses string follows ...\n\nand instead say\n\n\tconst char *string;\n\n\tstring = strchrnul(name, ':');\n\t... the real logic of the block that uses string follows ...\n"},{"id":"236068","messageId":"87eh2hi7jk.fsf@fencepost.gnu.org","threadId":"36046","inReplyTo":"xmqq8uspk72g.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] cache_tree_find(): remove redundant checks","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-03-05T04:38:39Z","receivedAt":"2014-03-05T04:38:39Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n>\n>>  \twhile (*path) {\n>> -\t\tconst char *slash;\n>>  \t\tstruct cache_tree_sub *sub;\n>> +\t\tconst char *slash = strchr(path, '/');\n>>  \n>> -\t\tslash = strchr(path, '/');\n>>  \t\tif (!slash)\n>>  \t\t\tslash = path + strlen(path);\n>\n> Isn't the above a strchrnul()?\n\nYes.  I realized that previously, but since it's a GNU extension rather\nthan part of the C standards, I discarded that idea.  Calling\n\n    git grep strchrnul\n\nshows, however, that it _is_ used plentifully already.\n\nThat would, indeed, favor the current proposal but with strchnul.\n\nStill worth thinking about whether there is no better name than slash\nfor something that indicated the end of the current path name segment.\n\n-- \nDavid Kastrup\n"},{"id":"236101","messageId":"xmqqeh2gil55.fsf@gitster.dls.corp.google.com","threadId":"36046","inReplyTo":"xmqqr46himd1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] cache_tree_find(): remove redundant checks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-05T17:57:10Z","receivedAt":"2014-03-05T17:57:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n>\n>> I really wish we could mix declarations with statements because I think\n>> it is a big help to readability.\n> ...\n> Unfortunately, I think we are in violent disagreement.\n\nAfter re-reading the above, I realize that my statement may have\nsounded a lot stronger than I intended it to.  If our codebase\nallowed decl-after-stmt, that would change the equation and a\ndifferent style might help readability somewhat.\n\nIf decl-after-stmt were allowed, the group of lines that declare\nvariables at the beginning before the real logic begins do not even\nhave to be there, and \"if some variables have initialization that\ninvolve program logic that need to be read carefully, the\ndeclaration at the beginning no longer can be coasted over as\nboilerplate\" complaint disappears.  The entire block can become the\nlogic, declaring variables as necessary at the point they are\nrequired, without having to have a separate decl at the beginning.\n\nNote that I am not advocating to allow decl-after-stmt; I do not\nthink the imagined readability benefit is worth it.\n"},{"id":"236108","messageId":"xmqq38iwij5j.fsf@gitster.dls.corp.google.com","threadId":"36046","inReplyTo":"87eh2hi7jk.fsf@fencepost.gnu.org","subject":"Re: [PATCH v2] cache_tree_find(): remove redundant checks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-05T18:40:08Z","receivedAt":"2014-03-05T18:40:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Kastrup <dak@gnu.org> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Michael Haggerty <mhagger@alum.mit.edu> writes:\n>>\n>>>  \twhile (*path) {\n>>> -\t\tconst char *slash;\n>>>  \t\tstruct cache_tree_sub *sub;\n>>> +\t\tconst char *slash = strchr(path, '/');\n>>>  \n>>> -\t\tslash = strchr(path, '/');\n>>>  \t\tif (!slash)\n>>>  \t\t\tslash = path + strlen(path);\n>>\n>> Isn't the above a strchrnul()?\n>\n> Yes.  I realized that previously, but since it's a GNU extension rather\n> than part of the C standards, I discarded that idea.  Calling\n>\n>     git grep strchrnul\n>\n> shows, however, that it _is_ used plentifully already.\n\nYes, we have a fallback definition in compat-util, I think.\n\n> Still worth thinking about whether there is no better name than slash\n> for something that indicated the end of the current path name segment.\n\nend_of_path_component?  Sounds a bit too long ;-)\n"}]}