Re: [PATCH 1/2] gettext API users: correct use of casts for Q_()

[Derrick Stolee <derrickstolee@github.com>]

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