{"thread":{"id":"36241","subject":"[PATCH] builtin/apply.c: fuzzy_matchlines:trying to fix some inefficiencies","startedAt":"2014-03-20T01:32:47Z","lastAt":"2014-03-20T10:58:12Z","messageCount":3,"participants":["George Papanikolaou","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"237139","messageId":"1395279167-20354-1-git-send-email-g3orge.app@gmail.com","threadId":"36241","inReplyTo":null,"subject":"[PATCH] builtin/apply.c: fuzzy_matchlines:trying to fix some inefficiencies","fromName":"George Papanikolaou","fromEmail":"g3orge.app@gmail.com","sentAt":"2014-03-20T01:32:47Z","receivedAt":"2014-03-20T01:32:47Z","isPatch":true,"sender":{"key":"g3orge.app@gmail.com","avatar":"https://gravatar.com/avatar/57d9756bb8e51dd138b79f2276e0bb0be2bfdc9aa60f08704de80e96d3a1c0f8?d=mp&s=160"},"body":"Hi fellows,\nI'm planning on applying on GSOC 2014...\n\nI tried my luck with that kinda weird microproject about inefficiencies,\nand I think I've discovered some.\n\n(also on a totally different mood, there are some warning about empty format\nstrings during compilation that could easily be silenced with some #pragma\ncalls on \"-Wformat-zero-length\". Is there a way you're not adding this?)\n\nThe empty buffers check could happen at the beggining.\nLeading whitespace check was unnecessary.\nSome style changes\n\nThanks.\n---\n builtin/apply.c | 25 +++++++++----------------\n 1 file changed, 9 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex b0d0986..df2435f 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -294,20 +294,16 @@ static int fuzzy_matchlines(const char *s1, size_t n1,\n \tconst char *last2 = s2 + n2 - 1;\n \tint result = 0;\n \n+\t/* early return if both lines are empty */\n+\tif ((s1 > last1) && (s2 > last2))\n+\t\treturn 1;\n+\n \t/* ignore line endings */\n \twhile ((*last1 == '\\r') || (*last1 == '\\n'))\n \t\tlast1--;\n \twhile ((*last2 == '\\r') || (*last2 == '\\n'))\n \t\tlast2--;\n \n-\t/* skip leading whitespace */\n-\twhile (isspace(*s1) && (s1 <= last1))\n-\t\ts1++;\n-\twhile (isspace(*s2) && (s2 <= last2))\n-\t\ts2++;\n-\t/* early return if both lines are empty */\n-\tif ((s1 > last1) && (s2 > last2))\n-\t\treturn 1;\n \twhile (!result) {\n \t\tresult = *s1++ - *s2++;\n \t\t/*\n@@ -315,18 +311,15 @@ static int fuzzy_matchlines(const char *s1, size_t n1,\n \t\t * both buffers because we don't want \"a b\" to match\n \t\t * \"ab\"\n \t\t */\n-\t\tif (isspace(*s1) && isspace(*s2)) {\n-\t\t\twhile (isspace(*s1) && s1 <= last1)\n-\t\t\t\ts1++;\n-\t\t\twhile (isspace(*s2) && s2 <= last2)\n-\t\t\t\ts2++;\n-\t\t}\n+\t\twhile (isspace(*s1) && s1 <= last1)\n+\t\t\ts1++;\n+\t\twhile (isspace(*s2) && s2 <= last2)\n+\t\t\ts2++;\n \t\t/*\n \t\t * If we reached the end on one side only,\n \t\t * lines don't match\n \t\t */\n-\t\tif (\n-\t\t    ((s2 > last2) && (s1 <= last1)) ||\n+\t\tif (((s2 > last2) && (s1 <= last1)) ||\n \t\t    ((s1 > last1) && (s2 <= last2)))\n \t\t\treturn 0;\n \t\tif ((s1 > last1) && (s2 > last2))\n-- \n1.9.0\n"},{"id":"237152","messageId":"532ABBE1.4090001@alum.mit.edu","threadId":"36241","inReplyTo":"1395279167-20354-1-git-send-email-g3orge.app@gmail.com","subject":"Re: [PATCH] builtin/apply.c: fuzzy_matchlines:trying to fix some inefficiencies","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-20T09:58:57Z","receivedAt":"2014-03-20T09:58:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Hello and welcome.  See the comments inline.\n\nOn 03/20/2014 02:32 AM, George Papanikolaou wrote:\n> Hi fellows,\n> I'm planning on applying on GSOC 2014...\n> \n> I tried my luck with that kinda weird microproject about inefficiencies,\n> and I think I've discovered some.\n> \n> (also on a totally different mood, there are some warning about empty format\n> strings during compilation that could easily be silenced with some #pragma\n> calls on \"-Wformat-zero-length\". Is there a way you're not adding this?)\n\nThe main reason is probably that #pragmas are compiler-specific.  It is\nundesirable to clutter up the source code with ugly #pragmas that only\nbenefit people using gcc.\n\nI think that most people who use gcc compile with\n-Wno-format-zero-length.  FWIW, the options that I use are\n\n    O = 2\n    CFLAGS = -g -O$(O) -Wall -Werror -Wdeclaration-after-statement\n-Wno-format-zero-length -Wno-format-security $(EXTRA_CFLAGS)\n\n, which you can put in your config.mak.\n\n> The empty buffers check could happen at the beggining.\n\ns/beggining/beginning/\n\n> Leading whitespace check was unnecessary.\n> Some style changes\n> \n> Thanks.\n> ---\n\nPlease pay attention to how patches have to be formatted:\n\nThe subject of the email and everything above the \"---\" line is used as\nthe commit's log message.  This should only include information that\nbelongs in the Git project's permanent history, not incidental\ninformation like the fact that you are applying for GSoC.\n\nThe commit message *should* include an explanation of *why* you are\nmaking a change, any tradeoffs that might be involved, etc.\n\nThe commit message also *must* include a Signed-off-by line.\n\nOther discussion, not intended for the commit message, should be placed\ndirectly *under* the \"---\" line.\n\n>  builtin/apply.c | 25 +++++++++----------------\n>  1 file changed, 9 insertions(+), 16 deletions(-)\n> \n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index b0d0986..df2435f 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -294,20 +294,16 @@ static int fuzzy_matchlines(const char *s1, size_t n1,\n>  \tconst char *last2 = s2 + n2 - 1;\n>  \tint result = 0;\n>  \n> +\t/* early return if both lines are empty */\n> +\tif ((s1 > last1) && (s2 > last2))\n> +\t\treturn 1;\n> +\n\nWhy is this an improvement?  Do you expect this function to be called\noften for empty lines (as opposed, for example, to lines consisting\nsolely of whitespace characters)?\n\n>  \t/* ignore line endings */\n>  \twhile ((*last1 == '\\r') || (*last1 == '\\n'))\n>  \t\tlast1--;\n>  \twhile ((*last2 == '\\r') || (*last2 == '\\n'))\n>  \t\tlast2--;\n>  \n> -\t/* skip leading whitespace */\n> -\twhile (isspace(*s1) && (s1 <= last1))\n> -\t\ts1++;\n> -\twhile (isspace(*s2) && (s2 <= last2))\n> -\t\ts2++;\n> -\t/* early return if both lines are empty */\n> -\tif ((s1 > last1) && (s2 > last2))\n> -\t\treturn 1;\n>  \twhile (!result) {\n>  \t\tresult = *s1++ - *s2++;\n>  \t\t/*\n> @@ -315,18 +311,15 @@ static int fuzzy_matchlines(const char *s1, size_t n1,\n>  \t\t * both buffers because we don't want \"a b\" to match\n>  \t\t * \"ab\"\n>  \t\t */\n> -\t\tif (isspace(*s1) && isspace(*s2)) {\n> -\t\t\twhile (isspace(*s1) && s1 <= last1)\n> -\t\t\t\ts1++;\n> -\t\t\twhile (isspace(*s2) && s2 <= last2)\n> -\t\t\t\ts2++;\n> -\t\t}\n> +\t\twhile (isspace(*s1) && s1 <= last1)\n> +\t\t\ts1++;\n> +\t\twhile (isspace(*s2) && s2 <= last2)\n> +\t\t\ts2++;\n\nThe comment just above this change gives a justification for putting an\n\"if\" statement surrounding the \"while\" statements.  Do you think the\ncomment's argument is incorrect?  If so, please explain why, and remove\nor change the comment.\n\n>  \t\t/*\n>  \t\t * If we reached the end on one side only,\n>  \t\t * lines don't match\n>  \t\t */\n> -\t\tif (\n> -\t\t    ((s2 > last2) && (s1 <= last1)) ||\n> +\t\tif (((s2 > last2) && (s1 <= last1)) ||\n\nThis reformatting doesn't have anything to do with the rest of your\npatch.  If it were important enough to make this change, then it should\nbe submitted as a separate patch.  But it is probably not important\nenough, and is only code churn, so it should probably be omitted entirely.\n\n>  \t\t    ((s1 > last1) && (s2 <= last2)))\n>  \t\t\treturn 0;\n>  \t\tif ((s1 > last1) && (s2 > last2))\n> \n\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"237164","messageId":"CAByyCQAsjoBiv54PR+AP=2ci60o39TNw5FhM0aNOhzbZpLd7gg@mail.gmail.com","threadId":"36241","inReplyTo":"532ABBE1.4090001@alum.mit.edu","subject":"Re: [PATCH] builtin/apply.c: fuzzy_matchlines:trying to fix some inefficiencies","fromName":"George Papanikolaou","fromEmail":"g3orge.app@gmail.com","sentAt":"2014-03-20T10:58:12Z","receivedAt":"2014-03-20T10:58:12Z","isPatch":true,"sender":{"key":"g3orge.app@gmail.com","avatar":"https://gravatar.com/avatar/57d9756bb8e51dd138b79f2276e0bb0be2bfdc9aa60f08704de80e96d3a1c0f8?d=mp&s=160"},"body":"Hi,\nThanks for the feedback,\n\nOn Thu, Mar 20, 2014 at 11:58 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>\n> Why is this an improvement?  Do you expect this function to be called\n> often for empty lines (as opposed, for example, to lines consisting\n> solely of whitespace characters)?\n>\n\nYes, you are probably right, we are not gonna get much (if any)\ncompletely empty lines\n\n>\n> The comment just above this change gives a justification for putting an\n> \"if\" statement surrounding the \"while\" statements.  Do you think the\n> comment's argument is incorrect?  If so, please explain why, and remove\n> or change the comment.\n>\n\nI see what I did wrong. I thought since that the if-condition is double checked\n(from the while clause) so I removed it.\n\nAlso this lead me to see that since the while clause is now unconditioned, there\nis no point of it being replicated exactly the same above, so I\nremoved that too. =(\n\nI'm trying to find other inefficiencies/irregularities on that\nfunction. I'm currently\nthinking on merging the first checks with a call to iswspace() or\nsomething similar.\n\nAlso thanks for clarifying the way patches/mails work.\n\nCheers.\n\n---\npapanikge's surrogate email.\nI may reply back.\nhttp://www.5slingshots.com/\n"}]}