threads / patch / 36313

patchMSVC: fix t0040-parse-options

Subject: [PATCH] MSVC: fix t0040-parse-options

## tl;dr

17 messages between Mar 28, 2014 and Mar 31, 2014. Diffs are folded; open one to read it.

replies: 16people: 5as markdown or json

Marat Radchenko· Mar 28, 2014, 12:04 UTC · lore
Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
---
 test-parse-options.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to test-parse-options.c +2 −1
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
Junio C Hamano· Mar 28, 2014, 18:19 UTC · re: Marat Radchenko · lore

Re: [PATCH] MSVC: fix t0040-parse-options

Marat Radchenko <marat@slonopotamus.org> writes:
Show 14 quoted lines
> 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?
Show 11 quoted lines
>  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(-)
Show changes to 2 files +2 −2

parse-options.h, test-parse-options.c

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"),
Marat Radchenko· Mar 29, 2014, 19:59 UTC · re: Junio C Hamano · lore

[PATCH v2] MSVC: fix t0040-parse-options crash

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(-)
Show changes to test-parse-options.c +1 −1
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
Marat Radchenko· Mar 29, 2014, 20:09 UTC · re: Marat Radchenko · lore

[PATCH v3] MSVC: fix t0040-parse-options crash

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(-)
Show changes to test-parse-options.c +1 −1
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
Andreas Schwab· Mar 29, 2014, 21:34 UTC · re: Marat Radchenko · lore

Re: [PATCH v3] MSVC: fix t0040-parse-options crash

Marat Radchenko <marat@slonopotamus.org> writes:
Show 10 quoted lines
> 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."
René Scharfe· Mar 29, 2014, 22:17 UTC · re: Andreas Schwab · lore

Re: [PATCH v3] MSVC: fix t0040-parse-options crash

Am 29.03.2014 22:34, schrieb Andreas Schwab:
Show 14 quoted lines
> 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é
Junio C Hamano· Mar 30, 2014, 02:01 UTC · re: Marat Radchenko · lore

Re: [PATCH v3] MSVC: fix t0040-parse-options crash

Marat Radchenko <marat@slonopotamus.org> writes:
Show 12 quoted lines
> 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.
Show 16 quoted lines
>  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(-)
Show changes to parse-options.h +1 −1
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) }
Andreas Schwab· Mar 30, 2014, 08:29 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] MSVC: fix t0040-parse-options crash

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."
Jeff King· Mar 31, 2014, 21:09 UTC · re: Andreas Schwab · lore

Re: [PATCH v3] MSVC: fix t0040-parse-options crash

On Sun, Mar 30, 2014 at 10:29:04AM +0200, Andreas Schwab wrote:
Show 9 quoted lines
> 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
Marat Radchenko· Mar 30, 2014, 11:08 UTC · re: Junio C Hamano · lore

[PATCH v4 0/3] Take four on fixing OPT_SET_PTR issues

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
Marat Radchenko· Mar 30, 2014, 11:08 UTC · re: Marat Radchenko · lore

[PATCH v4 1/3] MSVC: fix t0040-parse-options crash

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(-)
Show changes to test-parse-options.c +1 −1
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
Marat Radchenko· Mar 30, 2014, 11:08 UTC · re: Marat Radchenko · lore

[PATCH v4 2/3] parse-options: add cast to correct pointer type to OPT_SET_PTR

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(-)
Show changes to 2 files +2 −2

parse-options.h, test-parse-options.c

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
Junio C Hamano· Mar 31, 2014, 17:16 UTC · re: Marat Radchenko · lore

Re: [PATCH v4 2/3] parse-options: add cast to correct pointer type to OPT_SET_PTR

Marat Radchenko <marat@slonopotamus.org> writes:
Show 6 quoted lines
> 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>
;-)
Show 30 quoted lines
>  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"),
Marat Radchenko· Mar 30, 2014, 11:08 UTC · re: Marat Radchenko · lore

[PATCH v4 3/3] parse-options: remove unused OPT_SET_PTR

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(-)
Show changes to 5 files +4 −19

Documentation/technical/api-parse-options.txt, parse-options.c, parse-options.h, t/t0040-parse-options.sh, test-parse-options.c

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
Junio C Hamano· Mar 31, 2014, 17:23 UTC · re: Marat Radchenko · lore

Re: [PATCH v4 0/3] Take four on fixing OPT_SET_PTR issues

Marat Radchenko <marat@slonopotamus.org> writes:
Show 11 quoted lines
> 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.

Jeff King· Mar 31, 2014, 21:07 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 0/3] Take four on fixing OPT_SET_PTR issues

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
Junio C Hamano· Mar 31, 2014, 22:54 UTC · re: Jeff King · lore

Re: [PATCH v4 0/3] Take four on fixing OPT_SET_PTR issues

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
> 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.

← back to recent threads