On 3/7/2022 8:54 AM, Ævar Arnfjörð Bjarmason wrote:
>
> On Mon, Mar 07 2022, Derrick Stolee wrote:
>
>> On 3/7/2022 6:38 AM, Ævar Arnfjörð Bjarmason wrote:
>>> Change users of the inline gettext.h Q_() function to cast its
>>> argument to "unsigned long" instead of "int" or "unsigned int".
>>>
>>> The ngettext() function (which Q_() resolves to) takes an "unsigned
>>> long int", and so does our Q_() wrapper for it, see 0c9ea33b90f (i18n:
>>> add stub Q_() wrapper for ngettext, 2011-03-09).
>>>
>>> In a subsequent commit we'll be making more use of this pattern of:
>>>
>>> func(Q_(..%"PRIuMAX..., (unsigned long)x), (uintmax_t)x);
>>>
>>> By making this change we ensure that this case isn't the odd one out
>>> in that post-image.
>>
>>
>>> if (!res)
>>> - printf(Q_("updated %d path\n",
>>> - "updated %d paths\n", count), (int)count);
>>> + printf(Q_("updated %"PRIuMAX" path\n",
>>> + "updated %"PRIuMAX" paths\n", (unsigned long)count),
>>> + (uintmax_t)count);
>>
>> Why are we adding more uses of "unsigned long" which is not consistent
>> in its size across 64-bit Linux and 64-bit Windows? Specifically, on
>> Windows "unsigned long" is _not_ uintmax_t. Shouldn't we be using
>> uintmax_t everywhere instead?
>
> Whatever we do with "unsigned long" v.s. "size_t" or "uintmax_t" here
> we'll need to call the ngettext() function, which takes "unsigned long".
>
> Since you're quoting the part of the commit message that's explaining
> that I'm not sure if you're meaning this as a suggestion that the
> explanation should be clearer/more explicit, or just missed that
> ngettext() isn't ours...
Mostly missed which parts we had control over or not.
> I did wonder if we should just skip the casts here and instead do:
>
> diff --git a/gettext.h b/gettext.h
> index d209911ebb8..095bf6b0e5e 100644
> --- a/gettext.h
> +++ b/gettext.h
> @@ -49,8 +49,10 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)
> }
>
> static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)
> -const char *Q_(const char *msgid, const char *plu, unsigned long n)
> +const char *Q_(const char *msgid, const char *plu, size_t n)
> {
> + if (n > ULONG_MAX)
> + n = ULONG_MAX;
> return ngettext(msgid, plu, n);
> }
>
> Which I suppose would be more correct than a cast, but the edge case
> where we'd need that ULONG_MAX is so obscure that I don't think it's
> worth worrying about it.
>
> I think for this series it probably makes more sense to drop the casts
> for the "n" argument entirely, what do you think?
I agree that this is a better approach. It avoids polluting all of
the callers with down-casts and instead puts it here. The only
difference is that we will truncate at ULONG_MAX instead of blindly
ignoring the significant bits, which is probably better.
Thanks,
-Stolee
