[Xen-devel,01/20] xen/const: Introduce _BITUL and _BITULL

Message ID 20190422164937.21350-2-julien.grall@arm.com
State New
Headers show
Series
  • xen/arm: Clean-up & fixes in boot/mm code
Related show

Commit Message

Julien Grall April 22, 2019, 4:49 p.m.
The pattern _AC(1, UL{,L}) << X is commonly used in the headers to make
define usuable in both assembly and C.

So introduce _BITUL and _BITULL to make the code slightly more readable.

The idea has been taken from Linux (see include/uapi/linux.h).

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/include/xen/const.h | 3 +++
 1 file changed, 3 insertions(+)

Comments

Jan Beulich April 25, 2019, 12:15 p.m. | #1
>>> On 22.04.19 at 18:49, <julien.grall@arm.com> wrote:
> The pattern _AC(1, UL{,L}) << X is commonly used in the headers to make
> define usuable in both assembly and C.
> 
> So introduce _BITUL and _BITULL to make the code slightly more readable.

I don't particularly like the names, and I specifically object to
the leading underscores. I'm afraid I don't have better
suggestions for the names, but what I'd like to ask for is that
at least the UL / ULL be somehow separated from BIT. One
option might be something like

#define BIT(pos, sfx) (_AC(1, sfx) << (pos))

albeit BIT may be a little too generic a name, yet something
like DEFINE_BIT looks a little longish. But at least it would also
allow e.g. plain unsigned (non-long) constants to be defined
without yet another new construct.

Jan
Julien Grall April 29, 2019, 4:47 p.m. | #2
Hi,

On 25/04/2019 13:15, Jan Beulich wrote:
>>>> On 22.04.19 at 18:49, <julien.grall@arm.com> wrote:
>> The pattern _AC(1, UL{,L}) << X is commonly used in the headers to make
>> define usuable in both assembly and C.
>>
>> So introduce _BITUL and _BITULL to make the code slightly more readable.
> 
> I don't particularly like the names, and I specifically object to
> the leading underscores. I'm afraid I don't have better
> suggestions for the names, but what I'd like to ask for is that
> at least the UL / ULL be somehow separated from BIT. One
> option might be something like

The _ match the other assembly macro we have defined in const.h. I understand 
you don't like the leading underscores, but I think consistency is better here.

The more keeping the same generic naming lower down the churn to import code 
from Linux.

> 
> #define BIT(pos, sfx) (_AC(1, sfx) << (pos))

BIT() is already define in Xen and used in code coming from Linux. I would 
rather not change the prototype for this reason.

> 
> albeit BIT may be a little too generic a name, yet something
> like DEFINE_BIT looks a little longish. But at least it would also
> allow e.g. plain unsigned (non-long) constants to be defined
> without yet another new construct.

See above the reason why those names.

Cheers,
Jan Beulich April 30, 2019, 6:57 a.m. | #3
>>> On 29.04.19 at 18:47, <julien.grall@arm.com> wrote:
> On 25/04/2019 13:15, Jan Beulich wrote:
>>>>> On 22.04.19 at 18:49, <julien.grall@arm.com> wrote:
>>> The pattern _AC(1, UL{,L}) << X is commonly used in the headers to make
>>> define usuable in both assembly and C.
>>>
>>> So introduce _BITUL and _BITULL to make the code slightly more readable.
>> 
>> I don't particularly like the names, and I specifically object to
>> the leading underscores. I'm afraid I don't have better
>> suggestions for the names, but what I'd like to ask for is that
>> at least the UL / ULL be somehow separated from BIT. One
>> option might be something like
> 
> The _ match the other assembly macro we have defined in const.h. I understand 
> you don't like the leading underscores, but I think consistency is better here.

Well, "don't like" sounds like personal taste, but it goes beyond
that: It's a name space violation. I'm all for consistency, but not
at the expense of violating what the language standard demands.
If consistency is a goal here, the other macro names should be
changed rather than introducing more offenders.

> The more keeping the same generic naming lower down the churn to import code 
> from Linux.

While this is a desirable goal, as long as Linux doesn't care about
name space violations we shouldn't follow them slavishly (or
establish firmly that we don#t care about name space violations
either).

>> #define BIT(pos, sfx) (_AC(1, sfx) << (pos))
> 
> BIT() is already define in Xen and used in code coming from Linux. I would 
> rather not change the prototype for this reason.

That's up for debate. My proposal is more flexible than what we
currently have. Whether that outweighs becoming incompatible
with Linux'es similarly named macro.

Talking of Linux: Now that I've looked, I can't really figure why
they have both BIT_ULL() (linux/bits.h) and BITULL (linux/const.h).
The former is clearly redundant with the latter (and BIT() with
BITUL()). I don't think we should follow that model. In fact I think
BIT() as proposed above is then really the best solution, covering
everything that's needed in one go.

Jan

Patch

diff --git a/xen/include/xen/const.h b/xen/include/xen/const.h
index 0d5b2c64f5..b4b067bffa 100644
--- a/xen/include/xen/const.h
+++ b/xen/include/xen/const.h
@@ -21,4 +21,7 @@ 
 #define _AT(T,X)	((T)(X))
 #endif
 
+#define _BITUL(x)	(_AC(1, UL) << (x))
+#define _BITULL(x)	(_AC(1, ULL) << (x))
+
 #endif /* __XEN_CONST_H__ */