Hi Robin,

On Thu, 2019-12-05 at 17:48 +0000, Robin Murphy wrote:

On 03/12/2019 11:47 am, Nicolas Saenz Julienne wrote:

quoted

Some users need to make sure their rounding function accepts and returns
64bit long variables regardless of the architecture. Sadly
roundup/rounddown_pow_two() takes and returns unsigned longs. It turns
out ilog2() already handles 32/64bit calculations properly, and being
the building block to the round functions we can rework them as a
wrapper around it.

Neat! Although all the additional ULL casts this introduces seem 
somewhat unwelcome - I suppose the (1ULL << (ilog2(n))) makes it 
effectively always return unsigned long long now. Might it make sense to 
cast the return value to typeof(n) to avoid this slightly non-obvious 
behaviour (and the associated churn)?

It might alleviate some of the churn alright but I don't think a cast is really
going to make the behaviour more obvious. Say your expression is a big mess,
you'll have to analyze it to infer the output type, keeping in mind things like
integer promotion. See this example, 'params->nelem_hint' and
'params->min_size' are u16:

	diff --git a/lib/rhashtable.c b/lib/rhashtable.c
	index bdb7e4cadf05..70908678c7a8 100644
	--- a/lib/rhashtable.c
	+++ b/lib/rhashtable.c
	@@ -950,7 +950,7 @@ static size_t rounded_hashtable_size(const struct rhashtable_params *params)

		if (params->nelem_hint)
			retsize = max(roundup_pow_of_two(params->nelem_hint * 4 / 3),
	-                             (unsigned long)params->min_size);
	+                             (unsigned long long)params->min_size);
		else
			retsize = max(HASH_DEFAULT_SIZE,
				      (unsigned long)params->min_size);

With a cast the patch will look like this:

	diff --git a/lib/rhashtable.c b/lib/rhashtable.c
	index bdb7e4cadf05..70908678c7a8 100644
	--- a/lib/rhashtable.c
	+++ b/lib/rhashtable.c
	@@ -950,7 +950,7 @@ static size_t rounded_hashtable_size(const struct rhashtable_params *params)

		if (params->nelem_hint)
			retsize = max(roundup_pow_of_two(params->nelem_hint * 4 / 3),
	-                             (unsigned long)params->min_size);
	+                             (int)params->min_size);
		else
			retsize = max(HASH_DEFAULT_SIZE,
				      (unsigned long)params->min_size);

To me it's even less obvious than with a fixed ULL.

My intuition tells me to keep it as similar as the old behaviour, at the
expense of the extra churn (which is not that different from the current status
quo anyway). That said, I'll be happy to change it.

Regards,
Nicolas