This section explains which format should be followed by header
inclusion guards via a drop-down list of rules.
No functional change.
Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
---
CODING_STYLE | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/CODING_STYLE b/CODING_STYLE
index 7f6e9ad065..87836c97d4 100644
--- a/CODING_STYLE
+++ b/CODING_STYLE
@@ -167,3 +167,22 @@ the end of files. It should be:
* indent-tabs-mode: nil
* End:
*/
+
+Header inclusion guards
+-----------------------
+
+Unless differently specified all header files should have proper inclusion
+guards in order to avoid being included multiple times.
+The following naming conventions have been devised:
+- 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 */
+
--
2.34.1On 01.07.2024 15:46, Alessandro Zucchelli wrote: > This section explains which format should be followed by header > inclusion guards via a drop-down list of rules. Ah, so this answers my earlier question regarding where the naming rules are spelled out. Yet why is this not much earlier in the series, /before/ anything trying to follow these rules? > --- a/CODING_STYLE > +++ b/CODING_STYLE > @@ -167,3 +167,22 @@ the end of files. It should be: > * indent-tabs-mode: nil > * End: > */ This footer is not just an example; it also serves that function here. While not strictly needed in this file, I think it should still remain last. > + > +Header inclusion guards > +----------------------- > + > +Unless differently specified all header files should have proper inclusion > +guards in order to avoid being included multiple times. > +The following naming conventions have been devised: > +- 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 */ GENERIC contradicts X86. Please try to avoid giving confusing / possibly misleading examples. > +- 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 */ I'm afraid I can't connect the example to the filename pattern given: The example has 4 components separated by 3 underscores, when the pattern suggests 5 and 4 respectively. > + Please avoid empty lines at the bottom of files. Having reached the end, I don't see common headers (the ones under xen/include/ in the tree) covered. I can only conclude that the odd INCLUDE_ prefixes I had asked about were derived from the "private headers" rule. Yet what's in xen/include/ aren't private headers. 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. Finally, it shouldn't be silently assumed that all name components are to be converted to upper case; everything wants spelling out imo. Jan
On Wed, 3 Jul 2024, Jan Beulich wrote: > On 01.07.2024 15:46, Alessandro Zucchelli wrote: > > This section explains which format should be followed by header > > inclusion guards via a drop-down list of rules. > > Ah, so this answers my earlier question regarding where the naming > rules are spelled out. Yet why is this not much earlier in the series, > /before/ anything trying to follow these rules? > > > --- a/CODING_STYLE > > +++ b/CODING_STYLE > > @@ -167,3 +167,22 @@ the end of files. It should be: > > * indent-tabs-mode: nil > > * End: > > */ > > This footer is not just an example; it also serves that function here. > While not strictly needed in this file, I think it should still remain > last. +1 > > +Header inclusion guards > > +----------------------- > > + > > +Unless differently specified all header files should have proper inclusion > > +guards in order to avoid being included multiple times. > > +The following naming conventions have been devised: > > +- 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 */ > > GENERIC contradicts X86. Please try to avoid giving confusing / possibly > misleading examples. For clarity, Jan means that GENERIC by definition is not arch-specific so GENERIC_X86 or GENERIC_ARM is a contradiction and it would be better not to use it as reference example for this rule > > +- 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 */ > > I'm afraid I can't connect the example to the filename pattern given: > The example has 4 components separated by 3 underscores, when the > pattern suggests 5 and 4 respectively. I read the above with <subdir> being optional. But yes it is unclear because the example should have both the header guard but also the related file path. In this case the file path corresponding to ASM_X86_DOMAIN_H would be arch/x86/include/asm/domain.h > Please avoid empty lines at the bottom of files. > > Having reached the end, I don't see common headers (the ones under > xen/include/ in the tree) covered. I can only conclude that the odd > INCLUDE_ prefixes I had asked about were derived from the "private > headers" rule. Yet what's in xen/include/ aren't private headers. Yeah. I proposed in a previous email to use: - xen/include/xen/<filename>.h -> XEN_<filename>_H - xen/include/xen/<subdir>/<filename>.h -> XEN_<subdir>_<filename>_H with <subdir> being optional. > 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." > Finally, it shouldn't be silently assumed that all name components are > to be converted to upper case; everything wants spelling out imo. +1
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. Jan
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
two directories like this could collide:
- arch/arm/arm64/lib/something.h -> LIB_SOMETHING_H
- arch/arm/arm32/lib/something.h -> LIB_SOMETHING_H
- arch/x86/lib/something.h -> LIB_SOMETHING_H
(Leaving aside that in this example it would not be an issue because the
three headers are not meant to be all included in the same file.)
Can we specify that <dir> should go all the way back to the arch/ or or
common or drivers directory?
- 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
We can rely on the filesystem paths to make sure to avoid collisions.
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. > two directories like this could collide: > > - arch/arm/arm64/lib/something.h -> LIB_SOMETHING_H > - arch/arm/arm32/lib/something.h -> LIB_SOMETHING_H > - arch/x86/lib/something.h -> LIB_SOMETHING_H > > (Leaving aside that in this example it would not be an issue because the > three headers are not meant to be all included in the same file.) > > Can we specify that <dir> should go all the way back to the arch/ or or > common or drivers directory? > > - 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 We can of course, so long as we're okay(ish) with the length and redundancy. As already indicated before, there are downsides to this. Yet a firm scheme with few rules has the benefit that it might even be possible to use a script to do all the guard adjustments. Jan > We can rely on the filesystem paths to make sure to avoid collisions.
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 > > two directories like this could collide: > > > > - arch/arm/arm64/lib/something.h -> LIB_SOMETHING_H > > - arch/arm/arm32/lib/something.h -> LIB_SOMETHING_H > > - arch/x86/lib/something.h -> LIB_SOMETHING_H > > > > (Leaving aside that in this example it would not be an issue because the > > three headers are not meant to be all included in the same file.) > > > > Can we specify that <dir> should go all the way back to the arch/ or or > > common or drivers directory? > > > > - 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 > > We can of course, so long as we're okay(ish) with the length and redundancy. As > already indicated before, there are downsides to this. Yet a firm scheme with > few rules has the benefit that it might even be possible to use a script to do > all the guard adjustments. I also like the firm scheme with few rules
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. Jan
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?
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. Jan
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
- 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
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. Jan > - 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
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 >
On 2024-07-19 17:21, Stefano Stabellini wrote: > 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. I think we are good right now, I will provide the patch series v5 with all the fixes and inclusion guards renamings soon. >> > - 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 >> -- Alessandro Zucchelli, B.Sc. Software Engineer, BUGSENG (https://bugseng.com)
On 2024-07-15 09:23, 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. > If we could come to an agreement, I will make the new version of the patch series with all the needed changes. -- Alessandro Zucchelli, B.Sc. Software Engineer, BUGSENG (https://bugseng.com)
© 2016 - 2024 Red Hat, Inc.