git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>,
	Todd Zullinger <tmz@pobox.com>,
	git@vger.kernel.org
Subject: Re: [PATCH] test-progress: fix test failures on big-endian systems
Date: Thu, 24 Oct 2019 13:55:49 -0400	[thread overview]
Message-ID: <20191024175549.GA12892@sigill.intra.peff.net> (raw)
In-Reply-To: <20191024172442.GM4348@szeder.dev>

On Thu, Oct 24, 2019 at 07:24:42PM +0200, SZEDER Gábor wrote:

> On Mon, Oct 21, 2019 at 09:52:11AM +0900, Junio C Hamano wrote:
> > I can sympathize, but I do not think it is worth inventing OPT_U64()
> > or adding "int total_i" whose value is assigned to "u64 total" after
> > parsing a command line arg with OPT_INTEGER() into the former.
> 
> I agree, we should wait for the first real use case where specifying a
> larger-than-32bit integer actually makes sense in practice.

Anybody who wants to refer to a file or memory size would want that. We
have a few cases in pack-objects already, but they use OPT_MAGNITUDE()
instead. Which makes sense. Anything we expect to be that gigantic would
want to have the shorthand to say "4G" or whatever.

(As a side note, I notice that OPT_MAGNITUDE uses "unsigned long", which
probably needs to be adjusted for Windows. Dealing with all of the
void-pointer type-punning fallout from that will be fun. :) ).

> It's output looks like this when applied to an older version without
> the big-endian fix upthread:
> 
>   potential error at apply.c:4982:26:
>     passing variable 'state -> p_context' of type 'unsigned int' to OPT_INTEGER
>     OPT_INTEGER expects an int

Looks like this found a lot of the same issues my "intp" conversion
found. My recollection is that mine found more, but it might just have
been that the compiler's warning output is rather verbose. TBH, I didn't
carefully catalog them; as soon as I saw the problem, I went to bed in
disgust. ;)

> diff --git a/contrib/coccinelle/parse-options.cocci b/contrib/coccinelle/parse-options.cocci
> new file mode 100644
> index 0000000000..e0cddef421
> --- /dev/null
> +++ b/contrib/coccinelle/parse-options.cocci
> @@ -0,0 +1,18 @@
> +@ optint @
> +identifier opts;
> +type T;
> +T var;
> +expression SHORT, LONG, HELP;
> +position p;
> +@@
> +struct option opts[] = { ..., OPT_INTEGER(SHORT, LONG, &var@p, HELP), ...};
> +
> +@ script:python @
> +p << optint.p;
> +var << optint.var;
> +vartype << optint.T;
> +@@

I avoided relying on the python interpreter for previous cocci patches,
since some builds don't have it (IIRC, the one in Debian experimental
doesn't, but that one has not graduated even to "unstable" after several
years, so maybe it's not worth worrying about).

If we do start using python, we might want to revisit 4d168e742a
(coccinelle: use <...> for function exclusion, 2018-08-28), which
describes a faster python alternative in the commit message.

-Peff

  reply	other threads:[~2019-10-24 17:55 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-31  6:37 [BUG]: Testsuite failures on big-endian targets John Paul Adrian Glaubitz
2019-07-31  7:17 ` Todd Zullinger
2019-10-19 21:38   ` John Paul Adrian Glaubitz
2019-10-19 23:37     ` [PATCH] test-progress: fix test failures on big-endian systems SZEDER Gábor
2019-10-19 23:55       ` John Paul Adrian Glaubitz
2019-10-20  0:19         ` Todd Zullinger
2019-10-21  0:52       ` Junio C Hamano
2019-10-21  3:21         ` Jeff King
2019-10-21  8:51           ` Junio C Hamano
2019-10-21 18:49             ` Jeff King
2019-10-23  1:58               ` Junio C Hamano
2019-10-24 17:24         ` SZEDER Gábor
2019-10-24 17:55           ` Jeff King [this message]
2019-10-20  0:26     ` [BUG]: Testsuite failures on big-endian targets Todd Zullinger

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191024175549.GA12892@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=glaubitz@physik.fu-berlin.de \
    --cc=szeder.dev@gmail.com \
    --cc=tmz@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).