{"thread":{"id":"50706","subject":"[GSOC][PATCH] Fixed an issue which contained extra unnecessary code","startedAt":"2019-03-10T14:31:37Z","lastAt":"2019-03-10T18:26:09Z","messageCount":3,"participants":["sushmaunnibhavi","Christian Couder","Thomas Gummerer"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"371075","messageId":"20190310143126.GA13588@hacker-queen","threadId":"50706","inReplyTo":null,"subject":"[GSOC][PATCH] Fixed an issue which contained extra unnecessary code","fromName":"sushmaunnibhavi","fromEmail":"sushmaunnibhavi425@gmail.com","sentAt":"2019-03-10T14:31:26Z","receivedAt":"2019-03-10T14:31:37Z","isPatch":true,"sender":{"key":"sushmaunnibhavi425@gmail.com","avatar":null},"body":"From 5a6c233c6bf0a35aca000b32b9e936a34532900a Mon Sep 17 00:00:00 2001\nFrom: sushmaunnibhavi \nDate: Sun, 10 Mar 2019 19:37:33 +0530\nSubject: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code\nSigned-off-by: Sushma Unnibhavi \n---\nSince '\\n' and '\\0' are char_len==1,it is not necessary to check if the char_len<=1.\n compat/regex/regexec.c | 5 -----\n 1 file changed, 5 deletions(-)\n\ndiff --git a/compat/regex/regexec.c b/compat/regex/regexec.c\nindex 1b5d89fd5e..df62597531 100644\n--- a/compat/regex/regexec.c\n+++ b/compat/regex/regexec.c\n@@ -3799,11 +3799,6 @@ check_node_accept_bytes (const re_dfa_t *dfa, int node_idx,\n char_len = re_string_char_size_at (input, str_idx);\n if (node->type == OP_PERIOD)\n {\n- if (char_len <= 1)\n-\treturn 0;\n- /* FIXME: I don't think this if is needed, as both '\\n'\n-\t and '\\0' are char_len == 1. */\n- /* '.' accepts any one character except the following two cases. */\n if ((!(dfa->syntax & RE_DOT_NEWLINE) &&\n \t re_string_byte_at (input, str_idx) == '\\n') ||\n \t ((dfa->syntax & RE_DOT_NOT_NULL) &&\n-- \n2.17.1\n\n"},{"id":"371082","messageId":"CAP8UFD03cjbWE1pKRj93BKxxQnNuAUPCHNxmtM8RqqfJO2nu-Q@mail.gmail.com","threadId":"50706","inReplyTo":"20190310143126.GA13588@hacker-queen","subject":"Re: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-10T18:10:33Z","receivedAt":"2019-03-10T18:10:49Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Mar 10, 2019 at 4:30 PM sushmaunnibhavi\n wrote:\n>\n> From 5a6c233c6bf0a35aca000b32b9e936a34532900a Mon Sep 17 00:00:00 2001\n> From: sushmaunnibhavi \n> Date: Sun, 10 Mar 2019 19:37:33 +0530\n> Subject: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code\n> Signed-off-by: Sushma Unnibhavi \n> ---\n> Since '\\n' and '\\0' are char_len==1,it is not necessary to check if the char_len<=1.\n> compat/regex/regexec.c | 5 -----\n> 1 file changed, 5 deletions(-)\n\nIt doesn't look like the patch is formatted correctly. I think that\nthe explanation line (\"Since '\\n' and '\\0' are...\") should be above\nthe line that contains your \"Signed-off-by: ...\" and there should be a\nblank line between those two lines.\n\nAlso we ask for an author name in the \"From: ...\" header that looks\nlike \"Firstname Lastname\". A simple way to do that would be to make it\nmatch the name in your \"Signed-off-by: ...\".\n\n> diff --git a/compat/regex/regexec.c b/compat/regex/regexec.c\n> index 1b5d89fd5e..df62597531 100644\n> --- a/compat/regex/regexec.c\n> +++ b/compat/regex/regexec.c\n> @@ -3799,11 +3799,6 @@ check_node_accept_bytes (const re_dfa_t *dfa, int node_idx,\n> char_len = re_string_char_size_at (input, str_idx);\n> if (node->type == OP_PERIOD)\n> {\n> - if (char_len <= 1)\n> - return 0;\n> - /* FIXME: I don't think this if is needed, as both '\\n'\n> - and '\\0' are char_len == 1. */\n> - /* '.' accepts any one character except the following two cases. */\n> if ((!(dfa->syntax & RE_DOT_NEWLINE) &&\n> re_string_byte_at (input, str_idx) == '\\n') ||\n> ((dfa->syntax & RE_DOT_NOT_NULL) &&\n\nThe code looks like:\n\n char_len = re_string_char_size_at (input, str_idx);\n if (node->type == OP_PERIOD)\n {\n if (char_len <= 1)\n return 0;\n /* FIXME: I don't think this if is needed, as both '\\n'\n and '\\0' are char_len == 1. */\n /* '.' accepts any one character except the following two cases. */\n if ((!(dfa->syntax & RE_DOT_NEWLINE) &&\n re_string_byte_at (input, str_idx) == '\\n') ||\n ((dfa->syntax & RE_DOT_NOT_NULL) &&\n re_string_byte_at (input, str_idx) == '\\0'))\n return 0;\n return char_len;\n }\n\nIf the byte at re_string_byte_at (input, str_idx) is indeed '\\n' or\n'\\0', then yeah probably char_len == 1 so the current code should\nreturn 0 just before the code below the FIXME comment is reached. So\nin this case the 2 checks below the FIXME comment are useless because\nthey check re_string_byte_at (input, str_idx) == '\\n') and\nre_string_byte_at (input, str_idx) == '\\0' which cannot happen.\n\nSo I would say that the right fix would be to remove those 2 checks,\nnot to remove the if (char_len <= 1) check above the FIXME comment.\n\nAlso note that I used \"probably\" when I wrote \"then yeah probably\nchar_len == 1\", because I think it is worth checking what\nre_string_char_size_at() actually does before being too much\nconfident...\n"},{"id":"371083","messageId":"20190310182604.GG31533@hank.intra.tgummerer.com","threadId":"50706","inReplyTo":"20190310143126.GA13588@hacker-queen","subject":"Re: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-03-10T18:26:04Z","receivedAt":"2019-03-10T18:26:09Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"> Subject: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code\n\nCommit messages (and titles) should always be in the imperative mood.\nThe title in particular should be a short description of what the\npatch is doing, and should give meaningful information to people\nreading the output of 'git log --oneline'. Also we usually prefix the\ntitle with the area of the code we're touching.\n\nThe title above is very generic, and thus not very useful to future\nreaders. Something like\n\n compat/regex: remove unnecessary if\n\nwould give future readers a bit more context.\n\nOn 03/10, sushmaunnibhavi wrote:\n> From 5a6c233c6bf0a35aca000b32b9e936a34532900a Mon Sep 17 00:00:00 2001\n> From: sushmaunnibhavi \n> Date: Sun, 10 Mar 2019 19:37:33 +0530\n> Subject: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code\n\nThe four lines above don't need to be included here, as they are\nalready defined by the email headers.\n\nThe one line that may be useful here is \"From: sushmaunnibhavi\n\". That should be your full name, and\nmatch the name in the Signed-off-by line. Usually people just set\nthat up in the \"From\" header in the email, but if you are unable to\nconfigure your mailer like that, including a \"From: Sushma Unnibhavi\n\" before the rest of the commit message\nalso works.\n\n> Signed-off-by: Sushma Unnibhavi \n> ---\n> Since '\\n' and '\\0' are char_len==1,it is not necessary to check if the char_len<=1.\n\nThis explanation of why this is a good patch should be included in the\ncommit message before your Signed-off-by. If we want to apply this\nchange (which I don't think we want for the reasons stated below), I\nthink this needs a bit of a deeper analysis in the commit message, to\nconvince readers this is the correct thing to do.\n\nAlso please wrap commit messages at 72 characters.\n\n> compat/regex/regexec.c | 5 -----\n> 1 file changed, 5 deletions(-)\n> \n> diff --git a/compat/regex/regexec.c b/compat/regex/regexec.c\n\nThis file in compat/ was directly imported from upstream gawk. We\ngenerally don't patch this type of imported file, to make updates from\nupstream easier, unless there is an actual fix from upstream that\nneeds to be fixed that's not going to be fixed upstream.\n\nAs this change at best removes a redundant 'if' (I can't comment on the\ncorrectness, as I'm not familar with this code), so it is not worth\nchanging this file in our codebase.\n\n> index 1b5d89fd5e..df62597531 100644\n> --- a/compat/regex/regexec.c\n> +++ b/compat/regex/regexec.c\n> @@ -3799,11 +3799,6 @@ check_node_accept_bytes (const re_dfa_t *dfa, int node_idx,\n> char_len = re_string_char_size_at (input, str_idx);\n> if (node->type == OP_PERIOD)\n> {\n> - if (char_len <= 1)\n> -\treturn 0;\n> - /* FIXME: I don't think this if is needed, as both '\\n'\n> -\t and '\\0' are char_len == 1. */\n> - /* '.' accepts any one character except the following two cases. */\n> if ((!(dfa->syntax & RE_DOT_NEWLINE) &&\n> \t re_string_byte_at (input, str_idx) == '\\n') ||\n> \t ((dfa->syntax & RE_DOT_NOT_NULL) &&\n> -- \n> 2.17.1\n> \n"}]}