Re: [RFC PATCH 17/17] CODING_STYLE: Add a section on header guards naming conventions

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 19 Jul 2024, Jan Beulich wrote:
> On 19.07.2024 00:01, Stefano Stabellini wrote:
> > On Thu, 18 Jul 2024, Jan Beulich wrote:
> >> On 18.07.2024 01:02, Stefano Stabellini wrote:
> >>> On Wed, 17 Jul 2024, Jan Beulich wrote:
> >>>> On 17.07.2024 02:20, Stefano Stabellini wrote:
> >>>>> On Tue, 16 Jul 2024, Jan Beulich wrote:
> >>>>>> On 16.07.2024 02:43, Stefano Stabellini wrote:
> >>>>>>> On Mon, 15 Jul 2024, Jan Beulich wrote:
> >>>>>>>> On 13.07.2024 00:38, Stefano Stabellini wrote:
> >>>>>>>>> On Wed, 3 Jul 2024, Jan Beulich wrote:
> >>>>>>>>>> I further have to note that, as indicated during the earlier 
> >>>>>>>>>> discussion,
> >>>>>>>>>> I still cannot see how occasional ambiguity is going to be dealt 
> >>>>>>>>>> with.
> >>>>>>>>>> IOW from the rules above two different headers could still end up 
> >>>>>>>>>> with
> >>>>>>>>>> the same guard identifier.
> >>>>>>>>>
> >>>>>>>>> Maybe something like this?
> >>>>>>>>>
> >>>>>>>>> "In the event of naming collisions, exceptions to the coding style 
> >>>>>>>>> may
> >>>>>>>>> be made at the discretion of the contributor and maintainers."
> >>>>>>>>
> >>>>>>>> Hmm, maybe I wasn't clear enough then. My take is that the scheme 
> >>>>>>>> should
> >>>>>>>> simply not allow for possible collisions. Neither the contributor 
> >>>>>>>> nor the
> >>>>>>>> reviewer may spot such a collision, and it may therefore take until 
> >>>>>>>> the
> >>>>>>>> first full scan that one is actually noticed. Which I consider too 
> >>>>>>>> late
> >>>>>>>> in the process, even if we already were at the point where commits 
> >>>>>>>> were
> >>>>>>>> checked pre-push.
> >>>>>>>
> >>>>>>> Looking at the proposal, copy/pasted here for convenience:
> >>>>>>>
> >>>>>>> - private headers -> <dir>_<filename>_H
> >>>>>>> - asm-generic headers -> ASM_GENERIC_<filename>_H
> >>>>>>>     - #ifndef ASM_GENERIC_X86_PERCPU_H
> >>>>>>>       #define ASM_GENERIC_X86_PERCPU_H
> >>>>>>>       //...
> >>>>>>>       #endif /* ASM_GENERIC_X86_PERCPU_H */
> >>>>>>> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> 
> >>>>>>> ASM_<architecture>_<subdir>_<filename>_H
> >>>>>>>     - #ifndef ASM_X86_DOMAIN_H
> >>>>>>>       #define ASM_X86_DOMAIN_H
> >>>>>>>       //...
> >>>>>>>       #endif /* ASM_X86_DOMAIN_H */
> >>>>>>> - xen/include/xen/<filename>.h -> XEN_<filename>_H
> >>>>>>> - xen/include/xen/<subdir>/<filename>.h -> XEN_<subdir>_<filename>_H
> >>>>>>>
> >>>>>>>
> >>>>>>> The only possibility for collision that I can see is from the first
> >>>>>>> point:
> >>>>>>>
> >>>>>>> - private headers -> <dir>_<filename>_H
> >>>>>>
> >>>>>> I don't think this is the only possibility of collisions. The 
> >>>>>> <subdir>_<filename>
> >>>>>> parts can similarly cause problems if either of the two involved names 
> >>>>>> contains
> >>>>>> e.g. a dash (which would need converting to an underscore) or an 
> >>>>>> underscore. To
> >>>>>> avoid this, the name separators (slashes in the actual file names) 
> >>>>>> there may need
> >>>>>> representing by double underscores.
> >>>>>
> >>>>> I am OK with you two underscores as name separator (slashes in the
> >>>>> actual file names). Would you do it for all levels like this?
> >>>>>
> >>>>> - arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
> >>>>> - arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
> >>>>> - arch/x86/lib/something.h -> X86__LIB__SOMETHING_H
> >>>>>
> >>>>>
> >>>>> I think it is better than the below:
> >>>>>
> >>>>> - arch/arm/arm64/lib/something.h -> ARM_ARM64__LIB__SOMETHING_H
> >>>>> - arch/arm/arm32/lib/something.h -> ARM_ARM32__LIB__SOMETHING_H
> >>>>> - arch/x86/lib/something.h -> X86_LIB__SOMETHING_H
> >>>>
> >>>> Hmm, maybe it's indeed better to do it entirely uniformly then.
> >>>
> >>>
> >>> Do we have agreement on the naming convention then? 
> >>>
> >>>
> >>> - private headers -> <dir>__<filename>__H
> >>>     - arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
> >>>     - arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
> >>>     - arch/x86/lib/something.h -> X86__LIB__SOMETHING_H
> >>>
> >>> - asm-generic headers -> ASM_GENERIC_<filename>_H
> >>>     - include/asm-generic/percpu.h -> ASM_GENERIC_X86_PERCPU_H
> >>>
> >>> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> 
> >>> ASM_<architecture>_<subdir>_<filename>_H
> >>>     - arch/x86/include/asm/domain.h -> ASM_X86_DOMAIN_H
> >>>
> >>> - include/xen -> XEN_<filename>_H
> >>>     - include/xen/percpu.h -> XEN_PERCPU_H
> >>>
> >>>
> >>> Or do you prefer the double underscore __  in all cases?
> >>
> >> It's not so much prefer, but a requirement if we want to be future proof.
> >> Even for ASM_GENERIC_* that'll be needed, as your outline above simply
> >> doesn't mention the (future) case of there being subdir-s there (see how
> >> Linux already has some). Imo the question doesn't even arise for XEN_*,
> >> as xen/ has subdir-s already.
> > 
> > OK. So it becomes:
> > 
> > - private headers -> <dir>__<filename>_H
> >     - arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
> >     - arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
> >     - arch/x86/lib/something.h -> X86__LIB__SOMETHING_H
> > 
> > - asm-generic headers -> ASM_GENERIC__<filename>_H
> >     - include/asm-generic/percpu.h -> ASM_GENERIC__X86__PERCPU_H
> 
> Nit: There's still a stray _X86_ in here.
 
yes, good point.

Alessandro, let us know if we are good to go ahead or if we are missing
anything.


> > - arch/<architecture>/include/asm/<subdir>/<filename>.h -> 
> > ASM__<architecture>__<subdir>__<filename>_H
> >     - arch/x86/include/asm/domain.h -> ASM__X86__DOMAIN_H
> > 
> > - include/xen -> XEN__<filename>_H
> >     - include/xen/percpu.h -> XEN__PERCPU_H
> > 
> > If we have found agreement then Alessandro could send an update
>