|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH v2 00/10] address violations of MISRA C:2012 Directive 4.10
Hi Stefano, On 23/10/2023 21:47, Stefano Stabellini wrote: On Mon, 23 Oct 2023, Jan Beulich wrote:On 21.10.2023 01:26, Stefano Stabellini wrote:On Fri, 20 Oct 2023, Jan Beulich wrote:On 19.10.2023 18:19, Stefano Stabellini wrote:On Thu, 19 Oct 2023, Jan Beulich wrote:On 19.10.2023 02:44, Stefano Stabellini wrote:On Wed, 18 Oct 2023, Jan Beulich wrote:On 18.10.2023 02:48, Stefano Stabellini wrote:On Mon, 16 Oct 2023, Jan Beulich wrote:On 29.09.2023 00:24, Stefano Stabellini wrote:If it is not a MISRA requirement, then I think we should go for the path of least resistance and try to make the smallest amount of changes overall, which seems to be:... "least resistance" won't gain us much, as hardly any guards don't start with an underscore.- for xen/include/blah.h, __BLAH_H__ - for xen/arch/arm/asm/include/blah.h, __ASM_ARM_BLAH_H__ - for xen/arch/x86/asm/include/blah.h, it is far less consistent, maybe __ASM_X86_BLAH_H__ ?There are no headers in xen/include/. For (e.g.) xen/include/xen/ we may go with XEN_BLAH_H; whether ASM prefixes are needed I'm not sure; we could go with just ARM_BLAH_H and X86_BLAH_H? The primary question though is (imo) how to deal with private headers, such that the risk of name collisions is as small as possible.Looking at concrete examples under xen/include/xen: xen/include/xen/mm.h __XEN_MM_H__ xen/include/xen/dm.h __XEN_DM_H__ xen/include/xen/hypfs.h __XEN_HYPFS_H__ So I think we should do for consistency: xen/include/xen/blah.h __XEN_BLAH_H__ Even if we know the leading underscore are undesirable, in this case I would prefer consistency.I'm kind of okay with that. FTAOD - here and below you mean to make this one explicit first exception from the "no new leading underscores" goal, for newly added headers?Yes. The reason is for consistency with the existing header files.On the other hand looking at ARM examples: xen/arch/arm/include/asm/traps.h __ASM_ARM_TRAPS__ xen/arch/arm/include/asm/time.h __ARM_TIME_H__ xen/arch/arm/include/asm/sysregs.h __ASM_ARM_SYSREGS_H xen/arch/arm/include/asm/io.h _ASM_IO_H And also looking at x86 examples: xen/arch/x86/include/asm/paging.h _XEN_PAGING_H xen/arch/x86/include/asm/p2m.h _XEN_ASM_X86_P2M_H xen/arch/x86/include/asm/io.h _ASM_IO_H Thet are very inconsistent. So for ARM and X86 headers I think we are free to pick anything we want, including your suggested ARM_BLAH_H and X86_BLAH_H. Those are fine by me.To be honest, I'd prefer a global underlying pattern, i.e. if common headers are "fine" to use leading underscores for guards, arch header should, too.I am OK with that too. We could go with: __ASM_ARM_BLAH_H__ __ASM_X86_BLAH_H__ I used "ASM" to make it easier to differentiate with the private headers below. Also the version without "ASM" would work but it would only differ with the private headers in terms of leading underscores. I thought that also having "ASM" would help readability and help avoid confusion.For private headers such as: xen/arch/arm/vuart.h __ARCH_ARM_VUART_H__ xen/arch/arm/decode.h __ARCH_ARM_DECODE_H_ xen/arch/x86/mm/p2m.h __ARCH_MM_P2M_H__ xen/arch/x86/hvm/viridian/private.h X86_HVM_VIRIDIAN_PRIVATE_H More similar but still inconsistent. I would go with ARCH_ARM_BLAH_H and ARCH_X86_BLAH_H for new headers.I'm afraid I don't like this, as deeper paths would lead to unwieldy guard names. If we continue to use double-underscore prefixed names in common and arch headers, why don't we demand no leading underscores and no path-derived prefixes in private headers? That'll avoid any collisions between the two groups.OK, so for private headers: ARM_BLAH_H X86_BLAH_H What that work for you?What are the ARM_ and X86_ prefixes supposed to indicate here? Or to ask differently, how would you see e.g. common/decompress.h's guard named?I meant that: xen/arch/arm/blah.h would use ARM_BLAH_H xen/arch/x86/blah.h would use X86_BLAH_H You have a good question on something like xen/common/decompress.h and xen/common/event_channel.h. What do you think about: COMMON_BLAH_H, so specifically COMMON_DECOMPRESS_H otherwise: XEN_BLAH_H, so specifically XEN_DECOMPRESS_H I prefer COMMON_BLAH_H but I think both versions are OK.IOW you disagree with my earlier "... and no path-derived prefixes", and you prefer e.g. DRIVERS_PASSTHROUGH_VTD_DMAR_H as a consequence? FTAOD my earlier suggestion was simply based on the observation that the deeper the location of a header in the tree, the more unwieldy its guard name would end up being if path prefixes were to be used.I don't have a strong opinion on "path-derived prefixes". I prefer consistency and easy-to-figure-out guidelines over shortness. We adopted the MISRA Rule 5.4 which imposed us a limit (40 for Xen) on the number of characters for macros. AFAIU, this would apply to guards. In the example provided by Jan (DRIVERS_PASSTHROUGH_VTD_DMAR_H), this is already 31 characters. So this is quite close to the limit. The advantage of a path-derived prefix is that it is trivial to figure out at first glance. If we can come up with another system that is also easy then fine. Do you have a suggestion? If so, sorry I missed it.Well, I kind of implicitly suggested "no path derived prefixes for private headers", albeit realizing that there's a chance then of guards colliding. I can't think of a good scheme which would fit all goals (no collisions, uniformity, and not unduly long).Here I think we would benefit from a third opinion. Julien? Anyone? Just to confirm, the opinion is only for private headers. You have an agreement for the rest, is it correct? If so, then I think we need to have shorter names for guard to avoid hitting the 40 characters limit. I can't think of a way to have a common scheme between private and common headers. So I would consider to have a separate scheme. I am not sure if you or Jan already proposed an alternative scheme. Cheers, -- Julien Grall
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |