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. Jan
