* [PATCH] MSVC: fix t0040-parse-options @ 2014-03-28 12:04 Marat Radchenko 2014-03-28 18:19 ` Junio C Hamano 2014-03-29 20:09 ` [PATCH v3] " Marat Radchenko 0 siblings, 2 replies; 17+ messages in thread From: Marat Radchenko @ 2014-03-28 12:04 UTC (permalink / raw) To: git; +Cc: Marat Radchenko Signed-off-by: Marat Radchenko <marat@slonopotamus.org> --- test-parse-options.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test-parse-options.c b/test-parse-options.c index 434e8b8..7840493 100644 --- a/test-parse-options.c +++ b/test-parse-options.c @@ -11,6 +11,7 @@ static char *string = NULL; static char *file = NULL; static int ambiguous; static struct string_list list; +static const char *default_string = "default"; static int length_callback(const struct option *opt, const char *arg, int unset) { @@ -60,7 +61,7 @@ int main(int argc, char **argv) OPT_STRING('o', NULL, &string, "str", "get another string"), OPT_NOOP_NOARG(0, "obsolete"), OPT_SET_PTR(0, "default-string", &string, - "set string to default", (unsigned long)"default"), + "set string to default", default_string), OPT_STRING_LIST(0, "list", &list, "str", "add str to list"), OPT_GROUP("Magic arguments"), OPT_ARGUMENT("quux", "means --quux"), -- 1.9.1.501.gfbd1a76.dirty.MSVC ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] MSVC: fix t0040-parse-options 2014-03-28 12:04 [PATCH] MSVC: fix t0040-parse-options Marat Radchenko @ 2014-03-28 18:19 ` Junio C Hamano 2014-03-29 19:59 ` [PATCH v2] MSVC: fix t0040-parse-options crash Marat Radchenko 2014-03-29 20:09 ` [PATCH v3] " Marat Radchenko 1 sibling, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2014-03-28 18:19 UTC (permalink / raw) To: Marat Radchenko; +Cc: git Marat Radchenko <marat@slonopotamus.org> writes: > Signed-off-by: Marat Radchenko <marat@slonopotamus.org> > --- > test-parse-options.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/test-parse-options.c b/test-parse-options.c > index 434e8b8..7840493 100644 > --- a/test-parse-options.c > +++ b/test-parse-options.c > @@ -11,6 +11,7 @@ static char *string = NULL; > static char *file = NULL; > static int ambiguous; > static struct string_list list; > +static const char *default_string = "default"; That wastes 4 or 8 bytes compared to static const char default_string[] = "default"; no? > static int length_callback(const struct option *opt, const char *arg, int unset) > { > @@ -60,7 +61,7 @@ int main(int argc, char **argv) > OPT_STRING('o', NULL, &string, "str", "get another string"), > OPT_NOOP_NOARG(0, "obsolete"), > OPT_SET_PTR(0, "default-string", &string, > - "set string to default", (unsigned long)"default"), > + "set string to default", default_string), > OPT_STRING_LIST(0, "list", &list, "str", "add str to list"), > OPT_GROUP("Magic arguments"), > OPT_ARGUMENT("quux", "means --quux"), I can see how this patch would not hurt, but at the same time, I cannot see why this patch is a "FIX". A string literal "default" is a pointer to constant string, and being able to cast a pointer to "unsigned long" is something that is done fairly commonly without problems [*1*]. It needs to be explained why this change is needed along the lines of... We prepare an element in an array of "struct option" with OPT_SET_PTR to point a variable to a literal string "default", but MSVC compiler fails to distim the doshes for such and such reasons. Work it around by moving the literal string outside the definition of the struct option, which MSVC can understand it. in the log message. [Footnote] *1* The cast should actually be intptr_t for it to be kosher. I also suspect that the cast should happen inside OPT_SET_PTR() macro defintion, like in the attached patch. parse-options.h | 2 +- test-parse-options.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/parse-options.h b/parse-options.h index d670cb9..7a24d2e 100644 --- a/parse-options.h +++ b/parse-options.h @@ -129,7 +129,7 @@ struct option { #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \ (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1} #define OPT_SET_PTR(s, l, v, h, p) { OPTION_SET_PTR, (s), (l), (v), NULL, \ - (h), PARSE_OPT_NOARG, NULL, (p) } + (h), PARSE_OPT_NOARG, NULL, (intptr_t)(p) } #define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \ (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) } #define OPT_INTEGER(s, l, v, h) { OPTION_INTEGER, (s), (l), (v), N_("n"), (h) } diff --git a/test-parse-options.c b/test-parse-options.c index 434e8b8..10da63e 100644 --- a/test-parse-options.c +++ b/test-parse-options.c @@ -60,7 +60,7 @@ int main(int argc, char **argv) OPT_STRING('o', NULL, &string, "str", "get another string"), OPT_NOOP_NOARG(0, "obsolete"), OPT_SET_PTR(0, "default-string", &string, - "set string to default", (unsigned long)"default"), + "set string to default", "default"), OPT_STRING_LIST(0, "list", &list, "str", "add str to list"), OPT_GROUP("Magic arguments"), OPT_ARGUMENT("quux", "means --quux"), ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2] MSVC: fix t0040-parse-options crash 2014-03-28 18:19 ` Junio C Hamano @ 2014-03-29 19:59 ` Marat Radchenko 0 siblings, 0 replies; 17+ messages in thread From: Marat Radchenko @ 2014-03-29 19:59 UTC (permalink / raw) To: git; +Cc: Marat Radchenko, Junio C Hamano On 64-bit MSVC, pointers are 64 bit but `long` is only 32. Thus, casting string to `unsigned long`, which is redundand on other platforms, throws away important bits and when later cast to `intptr_t` results in corrupt pointer. This patch fixes test-parse-options by simply removing harming cast. Signed-off-by: Marat Radchenko <marat@slonopotamus.org> --- I will write verbose commit messages. I will write verbose commit messages. I will write verbose commit messages. I will write verbose commit messages. I will write verbose commit messages. I will write verbose commit messages. I will write verbose commit messages. I will write verbose commit messages. I will write verbose commit messages. I will write verbose commit messages. Junio, thank you for your patience. test-parse-options.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test-parse-options.c b/test-parse-options.c index 434e8b8..10da63e 100644 --- a/test-parse-options.c +++ b/test-parse-options.c @@ -60,7 +60,7 @@ int main(int argc, char **argv) OPT_STRING('o', NULL, &string, "str", "get another string"), OPT_NOOP_NOARG(0, "obsolete"), OPT_SET_PTR(0, "default-string", &string, - "set string to default", (unsigned long)"default"), + "set string to default", "default"), OPT_STRING_LIST(0, "list", &list, "str", "add str to list"), OPT_GROUP("Magic arguments"), OPT_ARGUMENT("quux", "means --quux"), -- 1.9.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v3] MSVC: fix t0040-parse-options crash 2014-03-28 12:04 [PATCH] MSVC: fix t0040-parse-options Marat Radchenko 2014-03-28 18:19 ` Junio C Hamano @ 2014-03-29 20:09 ` Marat Radchenko 2014-03-29 21:34 ` Andreas Schwab 2014-03-30 2:01 ` Junio C Hamano 1 sibling, 2 replies; 17+ messages in thread From: Marat Radchenko @ 2014-03-29 20:09 UTC (permalink / raw) To: git; +Cc: Marat Radchenko, Junio C Hamano On 64-bit MSVC, pointers are 64 bit but `long` is only 32. Thus, casting string to `unsigned long`, which is redundand on other platforms, throws away important bits and when later cast to `intptr_t` results in corrupt pointer. This patch fixes test-parse-options by replacing harming cast with correct one. Signed-off-by: Marat Radchenko <marat@slonopotamus.org> --- Aargh! Didn't notice that V2 introduced compilation warning. Take three. test-parse-options.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test-parse-options.c b/test-parse-options.c index 434e8b8..6f6c656 100644 --- a/test-parse-options.c +++ b/test-parse-options.c @@ -60,7 +60,7 @@ int main(int argc, char **argv) OPT_STRING('o', NULL, &string, "str", "get another string"), OPT_NOOP_NOARG(0, "obsolete"), OPT_SET_PTR(0, "default-string", &string, - "set string to default", (unsigned long)"default"), + "set string to default", (intptr_t)"default"), OPT_STRING_LIST(0, "list", &list, "str", "add str to list"), OPT_GROUP("Magic arguments"), OPT_ARGUMENT("quux", "means --quux"), -- 1.9.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3] MSVC: fix t0040-parse-options crash 2014-03-29 20:09 ` [PATCH v3] " Marat Radchenko @ 2014-03-29 21:34 ` Andreas Schwab 2014-03-29 22:17 ` René Scharfe 2014-03-30 2:01 ` Junio C Hamano 1 sibling, 1 reply; 17+ messages in thread From: Andreas Schwab @ 2014-03-29 21:34 UTC (permalink / raw) To: Marat Radchenko; +Cc: git, Junio C Hamano Marat Radchenko <marat@slonopotamus.org> writes: > diff --git a/test-parse-options.c b/test-parse-options.c > index 434e8b8..6f6c656 100644 > --- a/test-parse-options.c > +++ b/test-parse-options.c > @@ -60,7 +60,7 @@ int main(int argc, char **argv) > OPT_STRING('o', NULL, &string, "str", "get another string"), > OPT_NOOP_NOARG(0, "obsolete"), > OPT_SET_PTR(0, "default-string", &string, > - "set string to default", (unsigned long)"default"), > + "set string to default", (intptr_t)"default"), Why doesn't OPT_SET_PTR take a pointer? Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3] MSVC: fix t0040-parse-options crash 2014-03-29 21:34 ` Andreas Schwab @ 2014-03-29 22:17 ` René Scharfe 0 siblings, 0 replies; 17+ messages in thread From: René Scharfe @ 2014-03-29 22:17 UTC (permalink / raw) To: Andreas Schwab, Marat Radchenko; +Cc: git, Junio C Hamano Am 29.03.2014 22:34, schrieb Andreas Schwab: > Marat Radchenko <marat@slonopotamus.org> writes: > >> diff --git a/test-parse-options.c b/test-parse-options.c >> index 434e8b8..6f6c656 100644 >> --- a/test-parse-options.c >> +++ b/test-parse-options.c >> @@ -60,7 +60,7 @@ int main(int argc, char **argv) >> OPT_STRING('o', NULL, &string, "str", "get another string"), >> OPT_NOOP_NOARG(0, "obsolete"), >> OPT_SET_PTR(0, "default-string", &string, >> - "set string to default", (unsigned long)"default"), >> + "set string to default", (intptr_t)"default"), > > Why doesn't OPT_SET_PTR take a pointer? Good question. Here's another: OPT_SET_PTR (and OPTION_SET_PTR) has only ever been used by test-parse-options; can we remove it? René ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3] MSVC: fix t0040-parse-options crash 2014-03-29 20:09 ` [PATCH v3] " Marat Radchenko 2014-03-29 21:34 ` Andreas Schwab @ 2014-03-30 2:01 ` Junio C Hamano 2014-03-30 8:29 ` Andreas Schwab ` (4 more replies) 1 sibling, 5 replies; 17+ messages in thread From: Junio C Hamano @ 2014-03-30 2:01 UTC (permalink / raw) To: Marat Radchenko; +Cc: git Marat Radchenko <marat@slonopotamus.org> writes: > On 64-bit MSVC, pointers are 64 bit but `long` is only 32. > Thus, casting string to `unsigned long`, which is redundand on other > platforms, throws away important bits and when later cast to `intptr_t` > results in corrupt pointer. > > This patch fixes test-parse-options by replacing harming cast with > correct one. > > Signed-off-by: Marat Radchenko <marat@slonopotamus.org> > --- > > Aargh! Didn't notice that V2 introduced compilation warning. Take three. I am glad that I asked you to clarify, as I totally forgot that there are L32P64 boxes. I love it every time to see an attempt to describe why the solution works clearly results in a better patch. It is not about writing verbose log message; it is about thinking things through and clearly cut to the core of the issue. Moving the string literal to a separate variable to be used in the constructor in v1 was totally a red-herring. Your updated log message makes it crystal clear that using the correct typecast, not "unsigned long" but "intptr_t", is the core of the solution. As OPT_SET_PTR() is about setting the pointer value to intptr_t defval, a follow-up patch on top of this fix (see attached) may not be a bad thing to have, but that patch alone will not fix this issue without dropping the unneeded and unwanted cast to unsigned long. Thanks. > test-parse-options.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/test-parse-options.c b/test-parse-options.c > index 434e8b8..6f6c656 100644 > --- a/test-parse-options.c > +++ b/test-parse-options.c > @@ -60,7 +60,7 @@ int main(int argc, char **argv) > OPT_STRING('o', NULL, &string, "str", "get another string"), > OPT_NOOP_NOARG(0, "obsolete"), > OPT_SET_PTR(0, "default-string", &string, > - "set string to default", (unsigned long)"default"), > + "set string to default", (intptr_t)"default"), > OPT_STRING_LIST(0, "list", &list, "str", "add str to list"), > OPT_GROUP("Magic arguments"), > OPT_ARGUMENT("quux", "means --quux"), parse-options.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parse-options.h b/parse-options.h index d670cb9..7a24d2e 100644 --- a/parse-options.h +++ b/parse-options.h @@ -129,7 +129,7 @@ struct option { #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \ (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1} #define OPT_SET_PTR(s, l, v, h, p) { OPTION_SET_PTR, (s), (l), (v), NULL, \ - (h), PARSE_OPT_NOARG, NULL, (p) } + (h), PARSE_OPT_NOARG, NULL, (intptr_t)(p) } #define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \ (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) } #define OPT_INTEGER(s, l, v, h) { OPTION_INTEGER, (s), (l), (v), N_("n"), (h) } ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3] MSVC: fix t0040-parse-options crash 2014-03-30 2:01 ` Junio C Hamano @ 2014-03-30 8:29 ` Andreas Schwab 2014-03-31 21:09 ` Jeff King 2014-03-30 11:08 ` [PATCH v4 0/3] Take four on fixing OPT_SET_PTR issues Marat Radchenko ` (3 subsequent siblings) 4 siblings, 1 reply; 17+ messages in thread From: Andreas Schwab @ 2014-03-30 8:29 UTC (permalink / raw) To: Junio C Hamano; +Cc: Marat Radchenko, git Junio C Hamano <gitster@pobox.com> writes: > As OPT_SET_PTR() is about setting the pointer value to intptr_t defval, > a follow-up patch on top of this fix (see attached) may not be a bad > thing to have, but that patch alone will not fix this issue without > dropping the unneeded and unwanted cast to unsigned long. Wouldn't it make sense to change defval into a union to avoid the cast? (The intptr_t type may be too narrow for other values to be put there.) Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3] MSVC: fix t0040-parse-options crash 2014-03-30 8:29 ` Andreas Schwab @ 2014-03-31 21:09 ` Jeff King 0 siblings, 0 replies; 17+ messages in thread From: Jeff King @ 2014-03-31 21:09 UTC (permalink / raw) To: Andreas Schwab; +Cc: Junio C Hamano, Marat Radchenko, git On Sun, Mar 30, 2014 at 10:29:04AM +0200, Andreas Schwab wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > As OPT_SET_PTR() is about setting the pointer value to intptr_t defval, > > a follow-up patch on top of this fix (see attached) may not be a bad > > thing to have, but that patch alone will not fix this issue without > > dropping the unneeded and unwanted cast to unsigned long. > > Wouldn't it make sense to change defval into a union to avoid the cast? > (The intptr_t type may be too narrow for other values to be put there.) The primary function of these structs is to capture the information found in brace initializers. Is it possible in C89 to initialize the second member of a union (I think in C99, you can use named initializers). -Peff ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4 0/3] Take four on fixing OPT_SET_PTR issues 2014-03-30 2:01 ` Junio C Hamano 2014-03-30 8:29 ` Andreas Schwab @ 2014-03-30 11:08 ` Marat Radchenko 2014-03-31 17:23 ` Junio C Hamano 2014-03-30 11:08 ` [PATCH v4 1/3] MSVC: fix t0040-parse-options crash Marat Radchenko ` (2 subsequent siblings) 4 siblings, 1 reply; 17+ messages in thread From: Marat Radchenko @ 2014-03-30 11:08 UTC (permalink / raw) To: git; +Cc: Marat Radchenko, Junio C Hamano Patches summary: 1. Fix initial issue (incorrect cast causing crash on 64-bit MSVC) 2. Improve OPT_SET_PTR to prevent same errors in future 3. Purge OPT_SET_PTR away since nobody uses it *Optional* patch №3 is separated from №1 and №2 so that if someone someday decides to return OPT_SET_PTR back by reverting №3, it will be returned in a sane state. Decision of (not) merging №3 is left as an exercise to the reader due to my insufficient knowledge of accepted practices in Git project. Marat Radchenko (3): MSVC: fix t0040-parse-options crash parse-options: add cast to correct pointer type to OPT_SET_PTR parse-options: remove unused OPT_SET_PTR Documentation/technical/api-parse-options.txt | 4 ---- parse-options.c | 5 ----- parse-options.h | 5 +---- t/t0040-parse-options.sh | 7 +++---- test-parse-options.c | 2 -- 5 files changed, 4 insertions(+), 19 deletions(-) -- 1.9.0 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 0/3] Take four on fixing OPT_SET_PTR issues 2014-03-30 11:08 ` [PATCH v4 0/3] Take four on fixing OPT_SET_PTR issues Marat Radchenko @ 2014-03-31 17:23 ` Junio C Hamano 2014-03-31 21:07 ` Jeff King 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2014-03-31 17:23 UTC (permalink / raw) To: Marat Radchenko; +Cc: git Marat Radchenko <marat@slonopotamus.org> writes: > Patches summary: > 1. Fix initial issue (incorrect cast causing crash on 64-bit MSVC) > 2. Improve OPT_SET_PTR to prevent same errors in future > 3. Purge OPT_SET_PTR away since nobody uses it > > *Optional* patch №3 is separated from №1 and №2 so that if someone someday > decides to return OPT_SET_PTR back by reverting №3, it will be returned > in a sane state. > > Decision of (not) merging №3 is left as an exercise to the reader due to > my insufficient knowledge of accepted practices in Git project. SET_PTR() may not be used, but are there places where SET_INT() is abused with a cast-to-pointer for the same effect? I didn't check, but if there are such places, converting them to use SET_PTR() with their existing cast removed may be a better way to go. My suspicion is that there would be none, as switching the behaviour based on a small integer flag value is far easier than swapping the pointer to a pointee to be operated on, when responding to a command line option. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 0/3] Take four on fixing OPT_SET_PTR issues 2014-03-31 17:23 ` Junio C Hamano @ 2014-03-31 21:07 ` Jeff King 2014-03-31 22:54 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Jeff King @ 2014-03-31 21:07 UTC (permalink / raw) To: Junio C Hamano; +Cc: Marat Radchenko, git On Mon, Mar 31, 2014 at 10:23:44AM -0700, Junio C Hamano wrote: > SET_PTR() may not be used, but are there places where SET_INT() is > abused with a cast-to-pointer for the same effect? I didn't check, > but if there are such places, converting them to use SET_PTR() with > their existing cast removed may be a better way to go. Anyone doing that should be beaten with a clue stick. Fortunately, I grepped through and I did not see any cases. My clue stick remains untouched. -Peff ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 0/3] Take four on fixing OPT_SET_PTR issues 2014-03-31 21:07 ` Jeff King @ 2014-03-31 22:54 ` Junio C Hamano 0 siblings, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2014-03-31 22:54 UTC (permalink / raw) To: Jeff King; +Cc: Marat Radchenko, git Jeff King <peff@peff.net> writes: > On Mon, Mar 31, 2014 at 10:23:44AM -0700, Junio C Hamano wrote: > >> SET_PTR() may not be used, but are there places where SET_INT() is >> abused with a cast-to-pointer for the same effect? I didn't check, >> but if there are such places, converting them to use SET_PTR() with >> their existing cast removed may be a better way to go. > > Anyone doing that should be beaten with a clue stick. > > Fortunately, I grepped through and I did not see any cases. My clue > stick remains untouched. Yeah, I quickly did the same after sending the message out. Perhaps instead of taking all these three patches, it may be a good idea to just queue a single patch to remove both the feature and the "string (unset)" bit from the test. Thanks. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4 1/3] MSVC: fix t0040-parse-options crash 2014-03-30 2:01 ` Junio C Hamano 2014-03-30 8:29 ` Andreas Schwab 2014-03-30 11:08 ` [PATCH v4 0/3] Take four on fixing OPT_SET_PTR issues Marat Radchenko @ 2014-03-30 11:08 ` Marat Radchenko 2014-03-30 11:08 ` [PATCH v4 2/3] parse-options: add cast to correct pointer type to OPT_SET_PTR Marat Radchenko 2014-03-30 11:08 ` [PATCH v4 3/3] parse-options: remove unused OPT_SET_PTR Marat Radchenko 4 siblings, 0 replies; 17+ messages in thread From: Marat Radchenko @ 2014-03-30 11:08 UTC (permalink / raw) To: git; +Cc: Marat Radchenko, Junio C Hamano On 64-bit MSVC, pointers are 64 bit but `long` is only 32. Thus, casting string to `unsigned long`, which is redundand on other platforms, throws away important bits and when later cast to `intptr_t` results in corrupt pointer. This patch fixes test-parse-options by replacing harming cast with correct one. Signed-off-by: Marat Radchenko <marat@slonopotamus.org> --- test-parse-options.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test-parse-options.c b/test-parse-options.c index 434e8b8..6f6c656 100644 --- a/test-parse-options.c +++ b/test-parse-options.c @@ -60,7 +60,7 @@ int main(int argc, char **argv) OPT_STRING('o', NULL, &string, "str", "get another string"), OPT_NOOP_NOARG(0, "obsolete"), OPT_SET_PTR(0, "default-string", &string, - "set string to default", (unsigned long)"default"), + "set string to default", (intptr_t)"default"), OPT_STRING_LIST(0, "list", &list, "str", "add str to list"), OPT_GROUP("Magic arguments"), OPT_ARGUMENT("quux", "means --quux"), -- 1.9.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 2/3] parse-options: add cast to correct pointer type to OPT_SET_PTR 2014-03-30 2:01 ` Junio C Hamano ` (2 preceding siblings ...) 2014-03-30 11:08 ` [PATCH v4 1/3] MSVC: fix t0040-parse-options crash Marat Radchenko @ 2014-03-30 11:08 ` Marat Radchenko 2014-03-31 17:16 ` Junio C Hamano 2014-03-30 11:08 ` [PATCH v4 3/3] parse-options: remove unused OPT_SET_PTR Marat Radchenko 4 siblings, 1 reply; 17+ messages in thread From: Marat Radchenko @ 2014-03-30 11:08 UTC (permalink / raw) To: git; +Cc: Marat Radchenko, Junio C Hamano Do not force users of OPT_SET_PTR to cast pointer to correct underlying pointer type by integrating cast into OPT_SET_PTR macro. Cast is required to prevent 'initialization makes integer from pointer without a cast' compiler warning. --- parse-options.h | 2 +- test-parse-options.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/parse-options.h b/parse-options.h index 8fa02dc..54099d9 100644 --- a/parse-options.h +++ b/parse-options.h @@ -129,7 +129,7 @@ struct option { #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \ (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1} #define OPT_SET_PTR(s, l, v, h, p) { OPTION_SET_PTR, (s), (l), (v), NULL, \ - (h), PARSE_OPT_NOARG, NULL, (p) } + (h), PARSE_OPT_NOARG, NULL, (intptr_t)(p) } #define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \ (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) } #define OPT_INTEGER(s, l, v, h) { OPTION_INTEGER, (s), (l), (v), N_("n"), (h) } diff --git a/test-parse-options.c b/test-parse-options.c index 6f6c656..10da63e 100644 --- a/test-parse-options.c +++ b/test-parse-options.c @@ -60,7 +60,7 @@ int main(int argc, char **argv) OPT_STRING('o', NULL, &string, "str", "get another string"), OPT_NOOP_NOARG(0, "obsolete"), OPT_SET_PTR(0, "default-string", &string, - "set string to default", (intptr_t)"default"), + "set string to default", "default"), OPT_STRING_LIST(0, "list", &list, "str", "add str to list"), OPT_GROUP("Magic arguments"), OPT_ARGUMENT("quux", "means --quux"), -- 1.9.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/3] parse-options: add cast to correct pointer type to OPT_SET_PTR 2014-03-30 11:08 ` [PATCH v4 2/3] parse-options: add cast to correct pointer type to OPT_SET_PTR Marat Radchenko @ 2014-03-31 17:16 ` Junio C Hamano 0 siblings, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2014-03-31 17:16 UTC (permalink / raw) To: Marat Radchenko; +Cc: git Marat Radchenko <marat@slonopotamus.org> writes: > Do not force users of OPT_SET_PTR to cast pointer to correct > underlying pointer type by integrating cast into OPT_SET_PTR macro. > > Cast is required to prevent 'initialization makes integer from pointer > without a cast' compiler warning. > --- Signed-off-by (and probably "From:" too): Junio C Hamano <gitster@pobox.com> ;-) > parse-options.h | 2 +- > test-parse-options.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/parse-options.h b/parse-options.h > index 8fa02dc..54099d9 100644 > --- a/parse-options.h > +++ b/parse-options.h > @@ -129,7 +129,7 @@ struct option { > #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \ > (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1} > #define OPT_SET_PTR(s, l, v, h, p) { OPTION_SET_PTR, (s), (l), (v), NULL, \ > - (h), PARSE_OPT_NOARG, NULL, (p) } > + (h), PARSE_OPT_NOARG, NULL, (intptr_t)(p) } > #define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \ > (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) } > #define OPT_INTEGER(s, l, v, h) { OPTION_INTEGER, (s), (l), (v), N_("n"), (h) } > diff --git a/test-parse-options.c b/test-parse-options.c > index 6f6c656..10da63e 100644 > --- a/test-parse-options.c > +++ b/test-parse-options.c > @@ -60,7 +60,7 @@ int main(int argc, char **argv) > OPT_STRING('o', NULL, &string, "str", "get another string"), > OPT_NOOP_NOARG(0, "obsolete"), > OPT_SET_PTR(0, "default-string", &string, > - "set string to default", (intptr_t)"default"), > + "set string to default", "default"), > OPT_STRING_LIST(0, "list", &list, "str", "add str to list"), > OPT_GROUP("Magic arguments"), > OPT_ARGUMENT("quux", "means --quux"), ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4 3/3] parse-options: remove unused OPT_SET_PTR 2014-03-30 2:01 ` Junio C Hamano ` (3 preceding siblings ...) 2014-03-30 11:08 ` [PATCH v4 2/3] parse-options: add cast to correct pointer type to OPT_SET_PTR Marat Radchenko @ 2014-03-30 11:08 ` Marat Radchenko 4 siblings, 0 replies; 17+ messages in thread From: Marat Radchenko @ 2014-03-30 11:08 UTC (permalink / raw) To: git; +Cc: Marat Radchenko, Junio C Hamano OPT_SET_PTR was never used since its creation in 2007 (commit db7244bd5be12e389badb9cec621dbbcfa11f59a). --- Documentation/technical/api-parse-options.txt | 4 ---- parse-options.c | 5 ----- parse-options.h | 5 +---- t/t0040-parse-options.sh | 7 +++---- test-parse-options.c | 2 -- 5 files changed, 4 insertions(+), 19 deletions(-) diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt index be50cf4..1f2db31 100644 --- a/Documentation/technical/api-parse-options.txt +++ b/Documentation/technical/api-parse-options.txt @@ -160,10 +160,6 @@ There are some macros to easily define options: `int_var` is set to `integer` with `--option`, and reset to zero with `--no-option`. -`OPT_SET_PTR(short, long, &ptr_var, description, ptr)`:: - Introduce a boolean option. - If used, set `ptr_var` to `ptr`. - `OPT_STRING(short, long, &str_var, arg_str, description)`:: Introduce an option with string argument. The string argument is put into `str_var`. diff --git a/parse-options.c b/parse-options.c index c81d3a0..b536896 100644 --- a/parse-options.c +++ b/parse-options.c @@ -127,10 +127,6 @@ static int get_value(struct parse_opt_ctx_t *p, *(int *)opt->value = opt->defval; return 0; - case OPTION_SET_PTR: - *(void **)opt->value = unset ? NULL : (void *)opt->defval; - return 0; - case OPTION_STRING: if (unset) *(const char **)opt->value = NULL; @@ -367,7 +363,6 @@ static void parse_options_check(const struct option *opts) case OPTION_BIT: case OPTION_NEGBIT: case OPTION_SET_INT: - case OPTION_SET_PTR: case OPTION_NUMBER: if ((opts->flags & PARSE_OPT_OPTARG) || !(opts->flags & PARSE_OPT_NOARG)) diff --git a/parse-options.h b/parse-options.h index 54099d9..3189676 100644 --- a/parse-options.h +++ b/parse-options.h @@ -12,7 +12,6 @@ enum parse_opt_type { OPTION_NEGBIT, OPTION_COUNTUP, OPTION_SET_INT, - OPTION_SET_PTR, OPTION_CMDMODE, /* options with arguments (usually) */ OPTION_STRING, @@ -96,7 +95,7 @@ typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx, * * `defval`:: * default value to fill (*->value) with for PARSE_OPT_OPTARG. - * OPTION_{BIT,SET_INT,SET_PTR} store the {mask,integer,pointer} to put in + * OPTION_{BIT,SET_INT} store the {mask,integer,pointer} to put in * the value when met. * CALLBACKS can use it like they want. */ @@ -128,8 +127,6 @@ struct option { #define OPT_BOOL(s, l, v, h) OPT_SET_INT(s, l, v, h, 1) #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \ (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1} -#define OPT_SET_PTR(s, l, v, h, p) { OPTION_SET_PTR, (s), (l), (v), NULL, \ - (h), PARSE_OPT_NOARG, NULL, (intptr_t)(p) } #define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \ (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) } #define OPT_INTEGER(s, l, v, h) { OPTION_INTEGER, (s), (l), (v), N_("n"), (h) } diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index 65606df..4476548 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -30,7 +30,6 @@ String options --string2 <str> get another string --st <st> get another string (pervert ordering) -o <str> get another string - --default-string set string to default --list <str> add str to list Magic arguments @@ -293,7 +292,7 @@ cat > expect <<EOF boolean: 0 integer: 0 timestamp: 1 -string: default +string: (not set) abbrev: 7 verbose: 0 quiet: yes @@ -302,8 +301,8 @@ file: (not set) arg 00: foo EOF -test_expect_success 'OPT_DATE() and OPT_SET_PTR() work' ' - test-parse-options -t "1970-01-01 00:00:01 +0000" --default-string \ +test_expect_success 'OPT_DATE() work' ' + test-parse-options -t "1970-01-01 00:00:01 +0000" \ foo -q > output 2> output.err && test_must_be_empty output.err && test_cmp expect output diff --git a/test-parse-options.c b/test-parse-options.c index 10da63e..5dabce6 100644 --- a/test-parse-options.c +++ b/test-parse-options.c @@ -59,8 +59,6 @@ int main(int argc, char **argv) OPT_STRING(0, "st", &string, "st", "get another string (pervert ordering)"), OPT_STRING('o', NULL, &string, "str", "get another string"), OPT_NOOP_NOARG(0, "obsolete"), - OPT_SET_PTR(0, "default-string", &string, - "set string to default", "default"), OPT_STRING_LIST(0, "list", &list, "str", "add str to list"), OPT_GROUP("Magic arguments"), OPT_ARGUMENT("quux", "means --quux"), -- 1.9.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2014-03-31 22:54 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-03-28 12:04 [PATCH] MSVC: fix t0040-parse-options Marat Radchenko 2014-03-28 18:19 ` Junio C Hamano 2014-03-29 19:59 ` [PATCH v2] MSVC: fix t0040-parse-options crash Marat Radchenko 2014-03-29 20:09 ` [PATCH v3] " Marat Radchenko 2014-03-29 21:34 ` Andreas Schwab 2014-03-29 22:17 ` René Scharfe 2014-03-30 2:01 ` Junio C Hamano 2014-03-30 8:29 ` Andreas Schwab 2014-03-31 21:09 ` Jeff King 2014-03-30 11:08 ` [PATCH v4 0/3] Take four on fixing OPT_SET_PTR issues Marat Radchenko 2014-03-31 17:23 ` Junio C Hamano 2014-03-31 21:07 ` Jeff King 2014-03-31 22:54 ` Junio C Hamano 2014-03-30 11:08 ` [PATCH v4 1/3] MSVC: fix t0040-parse-options crash Marat Radchenko 2014-03-30 11:08 ` [PATCH v4 2/3] parse-options: add cast to correct pointer type to OPT_SET_PTR Marat Radchenko 2014-03-31 17:16 ` Junio C Hamano 2014-03-30 11:08 ` [PATCH v4 3/3] parse-options: remove unused OPT_SET_PTR Marat Radchenko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.