From: Phillip Wood Date: Fri, 18 Mar 2022 16:26:40 GMT Subject: Re: C99 %zu support (on MSVC) (was: [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref) Message-ID: In-Reply-To: <220318.86bky3cr8j.gmgdl@evledraar.gmail.com> On 18/03/2022 13:34, Ævar Arnfjörð Bjarmason wrote: > > On Wed, Mar 16 2022, brian m. carlson wrote: > >> [[PGP Signed Part:Undecided]] >> On 2022-03-14 at 21:19:10, Phillip Wood wrote: >>> Hi Brian and Ævar >>> >>> Firstly I think this is a useful feature to add to git stash, thanks for >>> working on it Brian >> >> Thanks. I'm glad folks other than me will find it useful. >> >>> On 11/03/2022 02:08, Ævar Arnfjörð Bjarmason wrote: >>>> >>>> On Thu, Mar 10 2022, brian m. carlson wrote: >>>> >>>>> + size_t author_len, committer_len; >>>>> + struct commit *this = NULL; >>>>> + const char *orig_author = NULL, *orig_committer = NULL; >>>>> + char *author = NULL, *committer = NULL; >>>>> + const char *buffer = NULL; >>>>> + unsigned long bufsize; >>>>> + const char *p; >>>>> + char *msg = NULL; >>>> >>>> These shouldn't be initialized unless they really need to.. >>>> >>>>> + this = lookup_commit_reference(the_repository, &info->w_commit); >>>> >>>> ..and some are clobbered right away here, so all of these should not be initializzed. >> >> This function got hoisted out of what would otherwise be duplicated >> code, and that's why they're all initialized (because we would otherwise >> have called free on an uninitialized value). I can remove the ones that >> aren't strictly needed. >> >>>>> + buffer = get_commit_buffer(this, &bufsize); >>>>> + orig_author = find_commit_header(buffer, "author", &author_len); >>>>> + orig_committer = find_commit_header(buffer, "committer", &committer_len); >>>>> + p = memmem(buffer, bufsize, "\n\n", 2); >>> >>> You could start searching from orig_committer rather than buffer but I'm >>> sure it doesn't make any real difference. The sequencer does something >>> similar to this to replay commits when rebasing - is there any scope for >>> sharing code between the two? >> >> I can look into it. The amount of code that would be duplicated here is >> very minimal, so I'm okay with just adding a few lines here. >> >>>> ...since by doing so we hide genuine "uninitialized" >>>> warnings. E.g. "author_len" here isn't initialized, but is set by >>>> find_commit_header(), but if that line was removed we'd warn below, but >>>> not if it's initialized when the variables are declared.. >>>> >>>>> + for (size_t i = 0;; i++, nitems++) { >>> >>> Do we need i and nitems? >> >> I can look into removing them. >> >>>>> + char buf[32]; >>>>> + int ret; >>>>> + >>>>> + if (nalloc <= i) { >>>>> + size_t new = nalloc * 3 / 2 + 5; >>>>> + items = xrealloc(items, new * sizeof(*items)); >>>>> + nalloc = new; >>>> >>>> Can't we just use the usual ALLOC_GROW() pattern here? >>> ALLOC_GROW_BY() zeros out the memory which would mean we could remove the >>> memset() calls in the loops. I noticed in some other loops we know the size >>> in advance and could use CALLOC_ARRAY(). >> >> Yeah, I can switch to that. I was looking for that, but I was thinking >> of a function and not a macro, so I missed it. >> >>>>> + } >>>>> + snprintf(buf, sizeof(buf), "%zu", i); >>>> >>>> Aren't the %z formats unportable (even with our newly found reliance on >>>> more C99)? I vaguely recall trying them recently and the windows CI jobs >>>> erroring... >>> >>> According to [1] it has been available since at least 2015. It is certainly >>> much nicer than casting every size_t to uintmax_t and having to use PRIuMAX. >> >> If we're relying on a new enough MSVC for C11, then it's much newer than >> 2015, so we should be fine. It's mandatory on POSIX systems. > > FWIW I dug into my logs and I ran into it with %zu (not %z), but that's > what you're using. > > Sorry about being inaccurate, it seems %z's portability isn't the same > as %z. > > I ran into it in mid-2021 in the GitHub CI, but those logs are deleted > now (and I didn't re-push that specific OID): > > https://github.com/avar/git/runs/2298653913 > > Where it would emit output like: > > builtin/log.c: In function 'gen_message_id': > 311 > builtin/log.c:1047:29: error: unknown conversion type character 'z' in format [-Werror=format=] > 312 > 1047 | strbuf_addf(&fmt, "%%s-%%0%zud.%%0%zud-%%s-%%s-%%s", tmp.len, tmp.len); > > This SO post, whose accuracy I can't verify, claims it is supported in > VS 2013 or later, and that the way to check for it is with "_MSC_VER >= > 1800": > > https://stackoverflow.com/questions/44382862/how-to-printf-a-size-t-without-warning-in-mingw-w64-gcc-7-1 Oh, so it is mingw that is the problem, not MSVC. Indeed using "%zu" (see the diff below) fails for the "win build" job[1] but the "win+VS build" succeeds[2]. mingw config.mak.uname to uses -D__MINGW_USE_ANSI_STDIO=0. Even if we could change that to =1 it would be insufficient as it does not affect the format specifiers allowed by __attribute__((format (printf, 3, 4))). I think to do that we would have to change "printf" to "__MINGW_PRINTF_FORMAT" for each attribute declaration[3]. In short unfortunately I don't think we can easily use "%zu" Best wishes Phillip diff --git a/add-interactive.c b/add-interactive.c index e1ab39cce3..1790ad6359 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -193,9 +193,8 @@ static ssize_t find_unique(const char *string, struct prefix_item_list *list) struct string_list_item *item; if (list->items.nr != list->sorted.nr) - BUG("prefix_item_list in inconsistent state (%"PRIuMAX - " vs %"PRIuMAX")", - (uintmax_t)list->items.nr, (uintmax_t)list->sorted.nr); + BUG("prefix_item_list in inconsistent state (%zu vs %zu)", + list->items.nr, list->sorted.nr); if (index < 0) item = list->sorted.items[-1 - index].util; [1] https://github.com/phillipwood/git/runs/5601840748?check_suite_focus=true [2] https://github.com/phillipwood/git/runs/5601840528?check_suite_focus=true [3] https://sourceforge.net/p/mingw-w64/wiki2/gnu%20printf > So if we are going to use it and that's true (which would be great!) it > would IMO make sense to introduce it in some prep commit where we delete > e.g. "_MSC_VER>=1310" and other things you'll see in-tree if you look > through: > > git grep _MSC_VER > > I.e. to push out some canary commit for using that specific feature, and > along with it delete the old MSVC compatibility shims (which presumably > we can't use if we're going to hard depend on %zu).