[PATCH] MSVC: fix t0040-parse-options

[PATCH] MSVC: fix t0040-parse-options

[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

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

[Junio C Hamano]

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"),

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

[Marat Radchenko]

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

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

[Marat Radchenko]

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

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

[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?

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."

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

[René Scharfe]

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é

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

[Junio C Hamano]

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) }

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

[Andreas Schwab]

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."

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

[Marat Radchenko]

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

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

[Marat Radchenko]

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

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

[Marat Radchenko]

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

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

[Marat Radchenko]

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

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

[Junio C Hamano]

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"),

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

[Junio C Hamano]

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.

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

[Jeff King]

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

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

[Jeff King]

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

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

[Junio C Hamano]

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.