Re: [PATCH v3 1/4] add-patch: Fix type conversion warnings from msvc

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 29, 2025, 19:52 UTC
Message-ID: <xmqqsep1iei6.fsf@gitster.g>
In-Reply-To: <6a251603-25bc-415d-ab8c-ae698bd7977a@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 7 quoted lines

> I'm afraid I'm still not convinced this is a good idea for the reasons
> I explained previously [1] together with an alternative approach to
> silencing these warnings. What makes "unsigned long" an incorrect
> choice when that's what "git diff" and "git apply" use?
>
> [1]
> https://lore.kernel.org/git/e396131c-1bd3-46d0-bae6-cd97ca9710d8@gmail.com

Ah, this patch still does that? I was hoping that it got corrected already after it was pointed out in the previous iterations. I agree with you that size_t is a dubious type to use for the line numbers there.

diff.c defines "struct emit_callback" with lno_in_{pre,post}image members that are of type "int", which is somewhat dubious, too, but at least we don't run on 16-bit machines, and being limited to 2 billion lines is probably OK. I am OK to upgrade that to long (if we use negative values for some oob signal) or ulong, but that is clearly outside of this topic.

Show 10 quoted lines

>> Add macro str_to_size_t for converting a string to size_t.
>> Test if convertion fails with over or underflow.
>
> That is welcome, but the implementation needs tweaking. If you look at
> other uses of strtoul() in our code you'll see that (somewhat
> unusually) one needs to set errno to zero before calling strtoul() as
> one cannot tell from the return value whether there was an error or
> not. As errno may have been set by a previous function call it needs
> to be cleared before calling strtoul() so we can be sure the error
> came from strtoul().

Nice advice.

> Best Wishes
>
> Phillip

Thanks.

By the way, who is <CAPig+cQ49Hdc_8=mRhhJDTny_Kqo6Wg6Nr98rsBN_YXmBrQ6kA@mail.gmail.com> and why is such an apparently bogus e-mail address Cc'ed?