{"thread":{"id":"36313","subject":"[PATCH] MSVC: fix t0040-parse-options","startedAt":"2014-03-28T12:04:58Z","lastAt":"2014-03-31T22:54:34Z","messageCount":17,"participants":["Marat Radchenko","Junio C Hamano","Andreas Schwab","René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"238001","messageId":"1396008298-1434-1-git-send-email-marat@slonopotamus.org","threadId":"36313","inReplyTo":null,"subject":"[PATCH] MSVC: fix t0040-parse-options","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2014-03-28T12:04:58Z","receivedAt":"2014-03-28T12:04:58Z","isPatch":true,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"Signed-off-by: Marat Radchenko \n---\n test-parse-options.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 434e8b8..7840493 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -11,6 +11,7 @@ static char *string = NULL;\n static char *file = NULL;\n static int ambiguous;\n static struct string_list list;\n+static const char *default_string = \"default\";\n \n static int length_callback(const struct option *opt, const char *arg, int unset)\n {\n@@ -60,7 +61,7 @@ int main(int argc, char **argv)\n \t\tOPT_STRING('o', NULL, &string, \"str\", \"get another string\"),\n \t\tOPT_NOOP_NOARG(0, \"obsolete\"),\n \t\tOPT_SET_PTR(0, \"default-string\", &string,\n-\t\t\t\"set string to default\", (unsigned long)\"default\"),\n+\t\t\t\"set string to default\", default_string),\n \t\tOPT_STRING_LIST(0, \"list\", &list, \"str\", \"add str to list\"),\n \t\tOPT_GROUP(\"Magic arguments\"),\n \t\tOPT_ARGUMENT(\"quux\", \"means --quux\"),\n-- \n1.9.1.501.gfbd1a76.dirty.MSVC\n"},{"id":"238025","messageId":"xmqq7g7eb2zv.fsf@gitster.dls.corp.google.com","threadId":"36313","inReplyTo":"1396008298-1434-1-git-send-email-marat@slonopotamus.org","subject":"Re: [PATCH] MSVC: fix t0040-parse-options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-28T18:19:00Z","receivedAt":"2014-03-28T18:19:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marat Radchenko writes:\n\n> Signed-off-by: Marat Radchenko \n> ---\n> test-parse-options.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/test-parse-options.c b/test-parse-options.c\n> index 434e8b8..7840493 100644\n> --- a/test-parse-options.c\n> +++ b/test-parse-options.c\n> @@ -11,6 +11,7 @@ static char *string = NULL;\n> static char *file = NULL;\n> static int ambiguous;\n> static struct string_list list;\n> +static const char *default_string = \"default\";\n\nThat wastes 4 or 8 bytes compared to\n\n\tstatic const char default_string[] = \"default\";\n\nno?\n\n> static int length_callback(const struct option *opt, const char *arg, int unset)\n> {\n> @@ -60,7 +61,7 @@ int main(int argc, char **argv)\n> \t\tOPT_STRING('o', NULL, &string, \"str\", \"get another string\"),\n> \t\tOPT_NOOP_NOARG(0, \"obsolete\"),\n> \t\tOPT_SET_PTR(0, \"default-string\", &string,\n> -\t\t\t\"set string to default\", (unsigned long)\"default\"),\n> +\t\t\t\"set string to default\", default_string),\n> \t\tOPT_STRING_LIST(0, \"list\", &list, \"str\", \"add str to list\"),\n> \t\tOPT_GROUP(\"Magic arguments\"),\n> \t\tOPT_ARGUMENT(\"quux\", \"means --quux\"),\n\nI can see how this patch would not hurt, but at the same time, I\ncannot see why this patch is a \"FIX\". A string literal \"default\" is\na pointer to constant string, and being able to cast a pointer to\n\"unsigned long\" is something that is done fairly commonly without\nproblems [*1*]. It needs to be explained why this change is needed\nalong the lines of...\n\n\tWe prepare an element in an array of \"struct option\" with\n\tOPT_SET_PTR to point a variable to a literal string\n\t\"default\", but MSVC compiler fails to distim the doshes for\n\tsuch and such reasons.\n\n Work it around by moving the literal string outside the\n\tdefinition of the struct option, which MSVC can understand\n\tit.\n\nin the log message.\n\n\n[Footnote]\n\n*1* The cast should actually be intptr_t for it to be kosher. I\n also suspect that the cast should happen inside OPT_SET_PTR()\n macro defintion, like in the attached patch.\n\n parse-options.h | 2 +-\n test-parse-options.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options.h b/parse-options.h\nindex d670cb9..7a24d2e 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -129,7 +129,7 @@ struct option {\n #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \\\n \t\t\t\t (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}\n #define OPT_SET_PTR(s, l, v, h, p) { OPTION_SET_PTR, (s), (l), (v), NULL, \\\n-\t\t\t\t (h), PARSE_OPT_NOARG, NULL, (p) }\n+\t\t\t\t (h), PARSE_OPT_NOARG, NULL, (intptr_t)(p) }\n #define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \\\n \t\t\t\t (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }\n #define OPT_INTEGER(s, l, v, h) { OPTION_INTEGER, (s), (l), (v), N_(\"n\"), (h) }\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 434e8b8..10da63e 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -60,7 +60,7 @@ int main(int argc, char **argv)\n \t\tOPT_STRING('o', NULL, &string, \"str\", \"get another string\"),\n \t\tOPT_NOOP_NOARG(0, \"obsolete\"),\n \t\tOPT_SET_PTR(0, \"default-string\", &string,\n-\t\t\t\"set string to default\", (unsigned long)\"default\"),\n+\t\t\t\"set string to default\", \"default\"),\n \t\tOPT_STRING_LIST(0, \"list\", &list, \"str\", \"add str to list\"),\n \t\tOPT_GROUP(\"Magic arguments\"),\n \t\tOPT_ARGUMENT(\"quux\", \"means --quux\"),\n"},{"id":"238057","messageId":"1396123198-26402-1-git-send-email-marat@slonopotamus.org","threadId":"36313","inReplyTo":"xmqq7g7eb2zv.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2] MSVC: fix t0040-parse-options crash","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2014-03-29T19:59:58Z","receivedAt":"2014-03-29T19:59:58Z","isPatch":true,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"On 64-bit MSVC, pointers are 64 bit but `long` is only 32.\nThus, casting string to `unsigned long`, which is redundand on other\nplatforms, throws away important bits and when later cast to `intptr_t`\nresults in corrupt pointer.\n\nThis patch fixes test-parse-options by simply removing harming cast.\n\nSigned-off-by: Marat Radchenko \n---\n\nI will write verbose commit messages. I will write verbose commit messages.\nI will write verbose commit messages. I will write verbose commit messages.\nI will write verbose commit messages. I will write verbose commit messages.\nI will write verbose commit messages. I will write verbose commit messages.\nI will write verbose commit messages. I will write verbose commit messages.\n\nJunio, thank you for your patience.\n\n test-parse-options.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 434e8b8..10da63e 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -60,7 +60,7 @@ int main(int argc, char **argv)\n \t\tOPT_STRING('o', NULL, &string, \"str\", \"get another string\"),\n \t\tOPT_NOOP_NOARG(0, \"obsolete\"),\n \t\tOPT_SET_PTR(0, \"default-string\", &string,\n-\t\t\t\"set string to default\", (unsigned long)\"default\"),\n+\t\t\t\"set string to default\", \"default\"),\n \t\tOPT_STRING_LIST(0, \"list\", &list, \"str\", \"add str to list\"),\n \t\tOPT_GROUP(\"Magic arguments\"),\n \t\tOPT_ARGUMENT(\"quux\", \"means --quux\"),\n-- \n1.9.0\n"},{"id":"238058","messageId":"1396123762-28673-1-git-send-email-marat@slonopotamus.org","threadId":"36313","inReplyTo":"1396008298-1434-1-git-send-email-marat@slonopotamus.org","subject":"[PATCH v3] MSVC: fix t0040-parse-options crash","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2014-03-29T20:09:22Z","receivedAt":"2014-03-29T20:09:22Z","isPatch":true,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"On 64-bit MSVC, pointers are 64 bit but `long` is only 32.\nThus, casting string to `unsigned long`, which is redundand on other\nplatforms, throws away important bits and when later cast to `intptr_t`\nresults in corrupt pointer.\n\nThis patch fixes test-parse-options by replacing harming cast with\ncorrect one.\n\nSigned-off-by: Marat Radchenko \n---\n\nAargh! Didn't notice that V2 introduced compilation warning. Take three.\n\n test-parse-options.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 434e8b8..6f6c656 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -60,7 +60,7 @@ int main(int argc, char **argv)\n \t\tOPT_STRING('o', NULL, &string, \"str\", \"get another string\"),\n \t\tOPT_NOOP_NOARG(0, \"obsolete\"),\n \t\tOPT_SET_PTR(0, \"default-string\", &string,\n-\t\t\t\"set string to default\", (unsigned long)\"default\"),\n+\t\t\t\"set string to default\", (intptr_t)\"default\"),\n \t\tOPT_STRING_LIST(0, \"list\", &list, \"str\", \"add str to list\"),\n \t\tOPT_GROUP(\"Magic arguments\"),\n \t\tOPT_ARGUMENT(\"quux\", \"means --quux\"),\n-- \n1.9.0\n"},{"id":"238059","messageId":"87ha6gpu2t.fsf@igel.home","threadId":"36313","inReplyTo":"1396123762-28673-1-git-send-email-marat@slonopotamus.org","subject":"Re: [PATCH v3] MSVC: fix t0040-parse-options crash","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2014-03-29T21:34:50Z","receivedAt":"2014-03-29T21:34:50Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Marat Radchenko writes:\n\n> diff --git a/test-parse-options.c b/test-parse-options.c\n> index 434e8b8..6f6c656 100644\n> --- a/test-parse-options.c\n> +++ b/test-parse-options.c\n> @@ -60,7 +60,7 @@ int main(int argc, char **argv)\n> \t\tOPT_STRING('o', NULL, &string, \"str\", \"get another string\"),\n> \t\tOPT_NOOP_NOARG(0, \"obsolete\"),\n> \t\tOPT_SET_PTR(0, \"default-string\", &string,\n> -\t\t\t\"set string to default\", (unsigned long)\"default\"),\n> +\t\t\t\"set string to default\", (intptr_t)\"default\"),\n\nWhy doesn't OPT_SET_PTR take a pointer?\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"238060","messageId":"53374667.3080605@web.de","threadId":"36313","inReplyTo":"87ha6gpu2t.fsf@igel.home","subject":"Re: [PATCH v3] MSVC: fix t0040-parse-options crash","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-03-29T22:17:11Z","receivedAt":"2014-03-29T22:17:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 29.03.2014 22:34, schrieb Andreas Schwab:\n> Marat Radchenko writes:\n>\n>> diff --git a/test-parse-options.c b/test-parse-options.c\n>> index 434e8b8..6f6c656 100644\n>> --- a/test-parse-options.c\n>> +++ b/test-parse-options.c\n>> @@ -60,7 +60,7 @@ int main(int argc, char **argv)\n>> \t\tOPT_STRING('o', NULL, &string, \"str\", \"get another string\"),\n>> \t\tOPT_NOOP_NOARG(0, \"obsolete\"),\n>> \t\tOPT_SET_PTR(0, \"default-string\", &string,\n>> -\t\t\t\"set string to default\", (unsigned long)\"default\"),\n>> +\t\t\t\"set string to default\", (intptr_t)\"default\"),\n>\n> Why doesn't OPT_SET_PTR take a pointer?\n\nGood question. Here's another: OPT_SET_PTR (and OPTION_SET_PTR) has \nonly ever been used by test-parse-options; can we remove it?\n\nRené\n"},{"id":"238070","messageId":"7vtxago359.fsf@alter.siamese.dyndns.org","threadId":"36313","inReplyTo":"1396123762-28673-1-git-send-email-marat@slonopotamus.org","subject":"Re: [PATCH v3] MSVC: fix t0040-parse-options crash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-30T02:01:54Z","receivedAt":"2014-03-30T02:01:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marat Radchenko writes:\n\n> On 64-bit MSVC, pointers are 64 bit but `long` is only 32.\n> Thus, casting string to `unsigned long`, which is redundand on other\n> platforms, throws away important bits and when later cast to `intptr_t`\n> results in corrupt pointer.\n>\n> This patch fixes test-parse-options by replacing harming cast with\n> correct one.\n>\n> Signed-off-by: Marat Radchenko \n> ---\n>\n> Aargh! Didn't notice that V2 introduced compilation warning. Take three.\n\nI am glad that I asked you to clarify, as I totally forgot that\nthere are L32P64 boxes.\n\nI love it every time to see an attempt to describe why the solution\nworks clearly results in a better patch. It is not about writing\nverbose log message; it is about thinking things through and clearly\ncut to the core of the issue. Moving the string literal to a\nseparate variable to be used in the constructor in v1 was totally a\nred-herring. Your updated log message makes it crystal clear that\nusing the correct typecast, not \"unsigned long\" but \"intptr_t\", is\nthe core of the solution.\n\nAs OPT_SET_PTR() is about setting the pointer value to intptr_t defval,\na follow-up patch on top of this fix (see attached) may not be a bad\nthing to have, but that patch alone will not fix this issue without\ndropping the unneeded and unwanted cast to unsigned long.\n\nThanks.\n\n> test-parse-options.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/test-parse-options.c b/test-parse-options.c\n> index 434e8b8..6f6c656 100644\n> --- a/test-parse-options.c\n> +++ b/test-parse-options.c\n> @@ -60,7 +60,7 @@ int main(int argc, char **argv)\n> \t\tOPT_STRING('o', NULL, &string, \"str\", \"get another string\"),\n> \t\tOPT_NOOP_NOARG(0, \"obsolete\"),\n> \t\tOPT_SET_PTR(0, \"default-string\", &string,\n> -\t\t\t\"set string to default\", (unsigned long)\"default\"),\n> +\t\t\t\"set string to default\", (intptr_t)\"default\"),\n> \t\tOPT_STRING_LIST(0, \"list\", &list, \"str\", \"add str to list\"),\n> \t\tOPT_GROUP(\"Magic arguments\"),\n> \t\tOPT_ARGUMENT(\"quux\", \"means --quux\"),\n\n\n parse-options.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/parse-options.h b/parse-options.h\nindex d670cb9..7a24d2e 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -129,7 +129,7 @@ struct option {\n #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \\\n \t\t\t\t (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}\n #define OPT_SET_PTR(s, l, v, h, p) { OPTION_SET_PTR, (s), (l), (v), NULL, \\\n-\t\t\t\t (h), PARSE_OPT_NOARG, NULL, (p) }\n+\t\t\t\t (h), PARSE_OPT_NOARG, NULL, (intptr_t)(p) }\n #define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \\\n \t\t\t\t (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }\n #define OPT_INTEGER(s, l, v, h) { OPTION_INTEGER, (s), (l), (v), N_(\"n\"), (h) }\n"},{"id":"238073","messageId":"m2wqfcm6nj.fsf@linux-m68k.org","threadId":"36313","inReplyTo":"7vtxago359.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] MSVC: fix t0040-parse-options crash","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2014-03-30T08:29:04Z","receivedAt":"2014-03-30T08:29:04Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Junio C Hamano writes:\n\n> As OPT_SET_PTR() is about setting the pointer value to intptr_t defval,\n> a follow-up patch on top of this fix (see attached) may not be a bad\n> thing to have, but that patch alone will not fix this issue without\n> dropping the unneeded and unwanted cast to unsigned long.\n\nWouldn't it make sense to change defval into a union to avoid the cast?\n(The intptr_t type may be too narrow for other values to be put there.)\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"238077","messageId":"cover.1396177207.git.marat@slonopotamus.org","threadId":"36313","inReplyTo":"7vtxago359.fsf@alter.siamese.dyndns.org","subject":"[PATCH v4 0/3] Take four on fixing OPT_SET_PTR issues","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2014-03-30T11:08:20Z","receivedAt":"2014-03-30T11:08:20Z","isPatch":true,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"Patches summary:\n1. Fix initial issue (incorrect cast causing crash on 64-bit MSVC)\n2. Improve OPT_SET_PTR to prevent same errors in future\n3. Purge OPT_SET_PTR away since nobody uses it\n\n*Optional* patch №3 is separated from №1 and №2 so that if someone someday\ndecides to return OPT_SET_PTR back by reverting №3, it will be returned\nin a sane state.\n\nDecision of (not) merging №3 is left as an exercise to the reader due to\nmy insufficient knowledge of accepted practices in Git project.\n\nMarat Radchenko (3):\n MSVC: fix t0040-parse-options crash\n parse-options: add cast to correct pointer type to OPT_SET_PTR\n parse-options: remove unused OPT_SET_PTR\n\n Documentation/technical/api-parse-options.txt | 4 ----\n parse-options.c | 5 -----\n parse-options.h | 5 +----\n t/t0040-parse-options.sh | 7 +++----\n test-parse-options.c | 2 --\n 5 files changed, 4 insertions(+), 19 deletions(-)\n\n-- \n1.9.0\n"},{"id":"238079","messageId":"cce7af835357635343f88895e8202cca149a701f.1396177208.git.marat@slonopotamus.org","threadId":"36313","inReplyTo":"cover.1396177207.git.marat@slonopotamus.org","subject":"[PATCH v4 1/3] MSVC: fix t0040-parse-options crash","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2014-03-30T11:08:21Z","receivedAt":"2014-03-30T11:08:21Z","isPatch":true,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"On 64-bit MSVC, pointers are 64 bit but `long` is only 32.\nThus, casting string to `unsigned long`, which is redundand on other\nplatforms, throws away important bits and when later cast to `intptr_t`\nresults in corrupt pointer.\n\nThis patch fixes test-parse-options by replacing harming cast with\ncorrect one.\n\nSigned-off-by: Marat Radchenko \n---\n test-parse-options.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 434e8b8..6f6c656 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -60,7 +60,7 @@ int main(int argc, char **argv)\n \t\tOPT_STRING('o', NULL, &string, \"str\", \"get another string\"),\n \t\tOPT_NOOP_NOARG(0, \"obsolete\"),\n \t\tOPT_SET_PTR(0, \"default-string\", &string,\n-\t\t\t\"set string to default\", (unsigned long)\"default\"),\n+\t\t\t\"set string to default\", (intptr_t)\"default\"),\n \t\tOPT_STRING_LIST(0, \"list\", &list, \"str\", \"add str to list\"),\n \t\tOPT_GROUP(\"Magic arguments\"),\n \t\tOPT_ARGUMENT(\"quux\", \"means --quux\"),\n-- \n1.9.0\n"},{"id":"238080","messageId":"55b78495a0a171d0dbe3ec5a39d04359e1989b91.1396177208.git.marat@slonopotamus.org","threadId":"36313","inReplyTo":"cover.1396177207.git.marat@slonopotamus.org","subject":"[PATCH v4 2/3] parse-options: add cast to correct pointer type to OPT_SET_PTR","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2014-03-30T11:08:22Z","receivedAt":"2014-03-30T11:08:22Z","isPatch":true,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"Do not force users of OPT_SET_PTR to cast pointer to correct\nunderlying pointer type by integrating cast into OPT_SET_PTR macro.\n\nCast is required to prevent 'initialization makes integer from pointer\nwithout a cast' compiler warning.\n---\n parse-options.h | 2 +-\n test-parse-options.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options.h b/parse-options.h\nindex 8fa02dc..54099d9 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -129,7 +129,7 @@ struct option {\n #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \\\n \t\t\t\t (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}\n #define OPT_SET_PTR(s, l, v, h, p) { OPTION_SET_PTR, (s), (l), (v), NULL, \\\n-\t\t\t\t (h), PARSE_OPT_NOARG, NULL, (p) }\n+\t\t\t\t (h), PARSE_OPT_NOARG, NULL, (intptr_t)(p) }\n #define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \\\n \t\t\t\t (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }\n #define OPT_INTEGER(s, l, v, h) { OPTION_INTEGER, (s), (l), (v), N_(\"n\"), (h) }\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 6f6c656..10da63e 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -60,7 +60,7 @@ int main(int argc, char **argv)\n \t\tOPT_STRING('o', NULL, &string, \"str\", \"get another string\"),\n \t\tOPT_NOOP_NOARG(0, \"obsolete\"),\n \t\tOPT_SET_PTR(0, \"default-string\", &string,\n-\t\t\t\"set string to default\", (intptr_t)\"default\"),\n+\t\t\t\"set string to default\", \"default\"),\n \t\tOPT_STRING_LIST(0, \"list\", &list, \"str\", \"add str to list\"),\n \t\tOPT_GROUP(\"Magic arguments\"),\n \t\tOPT_ARGUMENT(\"quux\", \"means --quux\"),\n-- \n1.9.0\n"},{"id":"238078","messageId":"a65dd5e1b030fcbc923752bfcee1c87acd6fb188.1396177208.git.marat@slonopotamus.org","threadId":"36313","inReplyTo":"cover.1396177207.git.marat@slonopotamus.org","subject":"[PATCH v4 3/3] parse-options: remove unused OPT_SET_PTR","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2014-03-30T11:08:23Z","receivedAt":"2014-03-30T11:08:23Z","isPatch":true,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"OPT_SET_PTR was never used since its creation in 2007 (commit\ndb7244bd5be12e389badb9cec621dbbcfa11f59a).\n---\n Documentation/technical/api-parse-options.txt | 4 ----\n parse-options.c | 5 -----\n parse-options.h | 5 +----\n t/t0040-parse-options.sh | 7 +++----\n test-parse-options.c | 2 --\n 5 files changed, 4 insertions(+), 19 deletions(-)\n\ndiff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\nindex be50cf4..1f2db31 100644\n--- a/Documentation/technical/api-parse-options.txt\n+++ b/Documentation/technical/api-parse-options.txt\n@@ -160,10 +160,6 @@ There are some macros to easily define options:\n \t`int_var` is set to `integer` with `--option`, and\n \treset to zero with `--no-option`.\n \n-`OPT_SET_PTR(short, long, &ptr_var, description, ptr)`::\n-\tIntroduce a boolean option.\n-\tIf used, set `ptr_var` to `ptr`.\n-\n `OPT_STRING(short, long, &str_var, arg_str, description)`::\n \tIntroduce an option with string argument.\n \tThe string argument is put into `str_var`.\ndiff --git a/parse-options.c b/parse-options.c\nindex c81d3a0..b536896 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -127,10 +127,6 @@ static int get_value(struct parse_opt_ctx_t *p,\n \t\t*(int *)opt->value = opt->defval;\n \t\treturn 0;\n \n-\tcase OPTION_SET_PTR:\n-\t\t*(void **)opt->value = unset ? NULL : (void *)opt->defval;\n-\t\treturn 0;\n-\n \tcase OPTION_STRING:\n \t\tif (unset)\n \t\t\t*(const char **)opt->value = NULL;\n@@ -367,7 +363,6 @@ static void parse_options_check(const struct option *opts)\n \t\tcase OPTION_BIT:\n \t\tcase OPTION_NEGBIT:\n \t\tcase OPTION_SET_INT:\n-\t\tcase OPTION_SET_PTR:\n \t\tcase OPTION_NUMBER:\n \t\t\tif ((opts->flags & PARSE_OPT_OPTARG) ||\n \t\t\t !(opts->flags & PARSE_OPT_NOARG))\ndiff --git a/parse-options.h b/parse-options.h\nindex 54099d9..3189676 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -12,7 +12,6 @@ enum parse_opt_type {\n \tOPTION_NEGBIT,\n \tOPTION_COUNTUP,\n \tOPTION_SET_INT,\n-\tOPTION_SET_PTR,\n \tOPTION_CMDMODE,\n \t/* options with arguments (usually) */\n \tOPTION_STRING,\n@@ -96,7 +95,7 @@ typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,\n *\n * `defval`::\n * default value to fill (*->value) with for PARSE_OPT_OPTARG.\n- * OPTION_{BIT,SET_INT,SET_PTR} store the {mask,integer,pointer} to put in\n+ * OPTION_{BIT,SET_INT} store the {mask,integer,pointer} to put in\n * the value when met.\n * CALLBACKS can use it like they want.\n */\n@@ -128,8 +127,6 @@ struct option {\n #define OPT_BOOL(s, l, v, h) OPT_SET_INT(s, l, v, h, 1)\n #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \\\n \t\t\t\t (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}\n-#define OPT_SET_PTR(s, l, v, h, p) { OPTION_SET_PTR, (s), (l), (v), NULL, \\\n-\t\t\t\t (h), PARSE_OPT_NOARG, NULL, (intptr_t)(p) }\n #define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \\\n \t\t\t\t (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }\n #define OPT_INTEGER(s, l, v, h) { OPTION_INTEGER, (s), (l), (v), N_(\"n\"), (h) }\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 65606df..4476548 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -30,7 +30,6 @@ String options\n --string2 get another string\n --st get another string (pervert ordering)\n -o get another string\n- --default-string set string to default\n --list add str to list\n \n Magic arguments\n@@ -293,7 +292,7 @@ cat > expect < output 2> output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 10da63e..5dabce6 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -59,8 +59,6 @@ int main(int argc, char **argv)\n \t\tOPT_STRING(0, \"st\", &string, \"st\", \"get another string (pervert ordering)\"),\n \t\tOPT_STRING('o', NULL, &string, \"str\", \"get another string\"),\n \t\tOPT_NOOP_NOARG(0, \"obsolete\"),\n-\t\tOPT_SET_PTR(0, \"default-string\", &string,\n-\t\t\t\"set string to default\", \"default\"),\n \t\tOPT_STRING_LIST(0, \"list\", &list, \"str\", \"add str to list\"),\n \t\tOPT_GROUP(\"Magic arguments\"),\n \t\tOPT_ARGUMENT(\"quux\", \"means --quux\"),\n-- \n1.9.0\n"},{"id":"238113","messageId":"xmqq8urq8f0j.fsf@gitster.dls.corp.google.com","threadId":"36313","inReplyTo":"55b78495a0a171d0dbe3ec5a39d04359e1989b91.1396177208.git.marat@slonopotamus.org","subject":"Re: [PATCH v4 2/3] parse-options: add cast to correct pointer type to OPT_SET_PTR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-31T17:16:44Z","receivedAt":"2014-03-31T17:16:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marat Radchenko writes:\n\n> Do not force users of OPT_SET_PTR to cast pointer to correct\n> underlying pointer type by integrating cast into OPT_SET_PTR macro.\n>\n> Cast is required to prevent 'initialization makes integer from pointer\n> without a cast' compiler warning.\n> ---\n\nSigned-off-by (and probably \"From:\" too): Junio C Hamano \n\n;-)\n\n> parse-options.h | 2 +-\n> test-parse-options.c | 2 +-\n> 2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/parse-options.h b/parse-options.h\n> index 8fa02dc..54099d9 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -129,7 +129,7 @@ struct option {\n> #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \\\n> \t\t\t\t (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}\n> #define OPT_SET_PTR(s, l, v, h, p) { OPTION_SET_PTR, (s), (l), (v), NULL, \\\n> -\t\t\t\t (h), PARSE_OPT_NOARG, NULL, (p) }\n> +\t\t\t\t (h), PARSE_OPT_NOARG, NULL, (intptr_t)(p) }\n> #define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \\\n> \t\t\t\t (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }\n> #define OPT_INTEGER(s, l, v, h) { OPTION_INTEGER, (s), (l), (v), N_(\"n\"), (h) }\n> diff --git a/test-parse-options.c b/test-parse-options.c\n> index 6f6c656..10da63e 100644\n> --- a/test-parse-options.c\n> +++ b/test-parse-options.c\n> @@ -60,7 +60,7 @@ int main(int argc, char **argv)\n> \t\tOPT_STRING('o', NULL, &string, \"str\", \"get another string\"),\n> \t\tOPT_NOOP_NOARG(0, \"obsolete\"),\n> \t\tOPT_SET_PTR(0, \"default-string\", &string,\n> -\t\t\t\"set string to default\", (intptr_t)\"default\"),\n> +\t\t\t\"set string to default\", \"default\"),\n> \t\tOPT_STRING_LIST(0, \"list\", &list, \"str\", \"add str to list\"),\n> \t\tOPT_GROUP(\"Magic arguments\"),\n> \t\tOPT_ARGUMENT(\"quux\", \"means --quux\"),\n"},{"id":"238114","messageId":"xmqq4n2e8eov.fsf@gitster.dls.corp.google.com","threadId":"36313","inReplyTo":"cover.1396177207.git.marat@slonopotamus.org","subject":"Re: [PATCH v4 0/3] Take four on fixing OPT_SET_PTR issues","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-31T17:23:44Z","receivedAt":"2014-03-31T17:23:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marat Radchenko writes:\n\n> Patches summary:\n> 1. Fix initial issue (incorrect cast causing crash on 64-bit MSVC)\n> 2. Improve OPT_SET_PTR to prevent same errors in future\n> 3. Purge OPT_SET_PTR away since nobody uses it\n>\n> *Optional* patch №3 is separated from №1 and №2 so that if someone someday\n> decides to return OPT_SET_PTR back by reverting №3, it will be returned\n> in a sane state.\n>\n> Decision of (not) merging №3 is left as an exercise to the reader due to\n> my insufficient knowledge of accepted practices in Git project.\n\nSET_PTR() may not be used, but are there places where SET_INT() is\nabused with a cast-to-pointer for the same effect? I didn't check,\nbut if there are such places, converting them to use SET_PTR() with\ntheir existing cast removed may be a better way to go.\n\nMy suspicion is that there would be none, as switching the behaviour\nbased on a small integer flag value is far easier than swapping the\npointer to a pointee to be operated on, when responding to a command\nline option.\n"},{"id":"238148","messageId":"20140331210714.GA6422@sigill.intra.peff.net","threadId":"36313","inReplyTo":"xmqq4n2e8eov.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 0/3] Take four on fixing OPT_SET_PTR issues","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-31T21:07:14Z","receivedAt":"2014-03-31T21:07:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 31, 2014 at 10:23:44AM -0700, Junio C Hamano wrote:\n\n> SET_PTR() may not be used, but are there places where SET_INT() is\n> abused with a cast-to-pointer for the same effect? I didn't check,\n> but if there are such places, converting them to use SET_PTR() with\n> their existing cast removed may be a better way to go.\n\nAnyone doing that should be beaten with a clue stick.\n\nFortunately, I grepped through and I did not see any cases. My clue\nstick remains untouched.\n\n-Peff\n"},{"id":"238149","messageId":"20140331210956.GB6422@sigill.intra.peff.net","threadId":"36313","inReplyTo":"m2wqfcm6nj.fsf@linux-m68k.org","subject":"Re: [PATCH v3] MSVC: fix t0040-parse-options crash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-31T21:09:56Z","receivedAt":"2014-03-31T21:09:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 30, 2014 at 10:29:04AM +0200, Andreas Schwab wrote:\n\n> Junio C Hamano writes:\n> \n> > As OPT_SET_PTR() is about setting the pointer value to intptr_t defval,\n> > a follow-up patch on top of this fix (see attached) may not be a bad\n> > thing to have, but that patch alone will not fix this issue without\n> > dropping the unneeded and unwanted cast to unsigned long.\n> \n> Wouldn't it make sense to change defval into a union to avoid the cast?\n> (The intptr_t type may be too narrow for other values to be put there.)\n\nThe primary function of these structs is to capture the information\nfound in brace initializers. Is it possible in C89 to initialize the\nsecond member of a union (I think in C99, you can use named\ninitializers).\n\n-Peff\n"},{"id":"238182","messageId":"xmqqy4zq0yj9.fsf@gitster.dls.corp.google.com","threadId":"36313","inReplyTo":"20140331210714.GA6422@sigill.intra.peff.net","subject":"Re: [PATCH v4 0/3] Take four on fixing OPT_SET_PTR issues","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-31T22:54:34Z","receivedAt":"2014-03-31T22:54:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Mar 31, 2014 at 10:23:44AM -0700, Junio C Hamano wrote:\n>\n>> SET_PTR() may not be used, but are there places where SET_INT() is\n>> abused with a cast-to-pointer for the same effect? I didn't check,\n>> but if there are such places, converting them to use SET_PTR() with\n>> their existing cast removed may be a better way to go.\n>\n> Anyone doing that should be beaten with a clue stick.\n>\n> Fortunately, I grepped through and I did not see any cases. My clue\n> stick remains untouched.\n\nYeah, I quickly did the same after sending the message out.\n\nPerhaps instead of taking all these three patches, it may be a good\nidea to just queue a single patch to remove both the feature and the\n\"string (unset)\" bit from the test.\n\nThanks.\n"}]}