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

[Junio C Hamano <gitster@pobox.com>]

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