| Message ID | 20190422164937.21350-2-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm: Clean-up & fixes in boot/mm code | expand |
>>> 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
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,
>>> 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
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__ */
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(+)