Re: [PATCH] xen: Give compile.h header guards
On Tue, 20 May 2025, Jan Beulich wrote: > On 19.05.2025 23:34, Stefano Stabellini wrote: > > On Mon, 19 May 2025, Jan Beulich wrote: > >> On 19.05.2025 15:52, Andrew Cooper wrote: > >>> Signed-off-by: Andrew Cooper > >> > >> Is this to please Misra in some way? > >> > >>> --- a/xen/include/xen/compile.h.in > >>> +++ b/xen/include/xen/compile.h.in > >>> @@ -1,3 +1,6 @@ > >>> +#ifndef XEN_COMPILE_H > >>> +#define XEN_COMPILE_H > >>> + > >>> #define XEN_COMPILE_DATE "@@date@@" > >>> #define XEN_COMPILE_TIME "@@time@@" > >>> #define XEN_COMPILE_BY "@@whoami@@" > >>> --- a/xen/tools/process-banner.sed > >>> +++ b/xen/tools/process-banner.sed > >>> @@ -12,3 +12,8 @@ s_(.*)_"\1\\n"_ > >>> > >>> # Trailing \ on all but the final line. > >>> $!s_$_ \\_ > >>> + > >>> +# Append closing header guard > >>> +$a\ > >>> +\ > >>> +#endif /* XEN_COMPILE_H */ > >> > >> This split of #ifndef and #endif is ugly. Can't we switch to something > >> more conventional, like > >> > >> #define XEN_BANNER "@@banner@@" > >> > >> with the first sed invocation then replacing this by the result of > >> a nested sed invocation using process-banner.sed (which of course would > >> need adjusting some)? (Maybe the double quotes would need omitting here, > >> for process-banner.sed to uniformly apply them.) > > > > While I agree with Jan that this is kind of ugly, it is a unique case > > and I would prefer an ugly but simple solution than a more complex > > solution. This would be different if we were talking about a widespread > > pattern, in which case the price for complexity would be worth it. > > > > My 2 cents in this case are in favor of the simplest approach. I would > > ack this patch. > > Feel free to do so; my comment was not meant as a plain objection, but more > as a suggestion. The one thing I would ask for is a non-empty description, > though. Fair enough. Andrew, please add a better commit message. With that: Reviewed-by: Stefano Stabellini
Re: [PATCH] xen: Give compile.h header guards
On Mon, May 19, 2025 at 2:52 PM Andrew Cooper wrote: > > Signed-off-by: Andrew Cooper > --- > CC: Anthony PERARD > CC: Michal Orzel > CC: Jan Beulich > CC: Julien Grall > CC: Roger Pau Monné > CC: Stefano Stabellini > --- > xen/include/xen/compile.h.in | 3 +++ > xen/tools/process-banner.sed | 5 + > 2 files changed, 8 insertions(+) > > diff --git a/xen/include/xen/compile.h.in b/xen/include/xen/compile.h.in > index 3151d1e7d1bf..9206341ba692 100644 > --- a/xen/include/xen/compile.h.in > +++ b/xen/include/xen/compile.h.in > @@ -1,3 +1,6 @@ > +#ifndef XEN_COMPILE_H > +#define XEN_COMPILE_H > + Why not follow CODING_STYLE ? OT: Maybe while on it, why not add SPDX comments too ? > #define XEN_COMPILE_DATE "@@date@@" > #define XEN_COMPILE_TIME "@@time@@" > #define XEN_COMPILE_BY "@@whoami@@" > diff --git a/xen/tools/process-banner.sed b/xen/tools/process-banner.sed > index 56c76558bcd9..4cf3f9a1163a 100755 > --- a/xen/tools/process-banner.sed > +++ b/xen/tools/process-banner.sed > @@ -12,3 +12,8 @@ s_(.*)_"\1\\n"_ > > # Trailing \ on all but the final line. > $!s_$_ \\_ > + > +# Append closing header guard > +$a\ > +\ > +#endif /* XEN_COMPILE_H */ > > base-commit: 6fc02ebdd053856221f37ba5306232ac1575332d > prerequisite-patch-id: 7bc1c498ba2c9c4a4939a56a0006f820f47f2a66 > -- > 2.39.5 > > Frediano
Re: [PATCH] xen: Give compile.h header guards
On 19.05.2025 23:34, Stefano Stabellini wrote: > On Mon, 19 May 2025, Jan Beulich wrote: >> On 19.05.2025 15:52, Andrew Cooper wrote: >>> Signed-off-by: Andrew Cooper >> >> Is this to please Misra in some way? >> >>> --- a/xen/include/xen/compile.h.in >>> +++ b/xen/include/xen/compile.h.in >>> @@ -1,3 +1,6 @@ >>> +#ifndef XEN_COMPILE_H >>> +#define XEN_COMPILE_H >>> + >>> #define XEN_COMPILE_DATE "@@date@@" >>> #define XEN_COMPILE_TIME "@@time@@" >>> #define XEN_COMPILE_BY "@@whoami@@" >>> --- a/xen/tools/process-banner.sed >>> +++ b/xen/tools/process-banner.sed >>> @@ -12,3 +12,8 @@ s_(.*)_"\1\\n"_ >>> >>> # Trailing \ on all but the final line. >>> $!s_$_ \\_ >>> + >>> +# Append closing header guard >>> +$a\ >>> +\ >>> +#endif /* XEN_COMPILE_H */ >> >> This split of #ifndef and #endif is ugly. Can't we switch to something >> more conventional, like >> >> #define XEN_BANNER "@@banner@@" >> >> with the first sed invocation then replacing this by the result of >> a nested sed invocation using process-banner.sed (which of course would >> need adjusting some)? (Maybe the double quotes would need omitting here, >> for process-banner.sed to uniformly apply them.) > > While I agree with Jan that this is kind of ugly, it is a unique case > and I would prefer an ugly but simple solution than a more complex > solution. This would be different if we were talking about a widespread > pattern, in which case the price for complexity would be worth it. > > My 2 cents in this case are in favor of the simplest approach. I would > ack this patch. Feel free to do so; my comment was not meant as a plain objection, but more as a suggestion. The one thing I would ask for is a non-empty description, though. Jan
Re: [PATCH] xen: Give compile.h header guards
On Mon, 19 May 2025, Jan Beulich wrote: > On 19.05.2025 15:52, Andrew Cooper wrote: > > Signed-off-by: Andrew Cooper > > Is this to please Misra in some way? > > > --- a/xen/include/xen/compile.h.in > > +++ b/xen/include/xen/compile.h.in > > @@ -1,3 +1,6 @@ > > +#ifndef XEN_COMPILE_H > > +#define XEN_COMPILE_H > > + > > #define XEN_COMPILE_DATE "@@date@@" > > #define XEN_COMPILE_TIME "@@time@@" > > #define XEN_COMPILE_BY "@@whoami@@" > > --- a/xen/tools/process-banner.sed > > +++ b/xen/tools/process-banner.sed > > @@ -12,3 +12,8 @@ s_(.*)_"\1\\n"_ > > > > # Trailing \ on all but the final line. > > $!s_$_ \\_ > > + > > +# Append closing header guard > > +$a\ > > +\ > > +#endif /* XEN_COMPILE_H */ > > This split of #ifndef and #endif is ugly. Can't we switch to something > more conventional, like > > #define XEN_BANNER"@@banner@@" > > with the first sed invocation then replacing this by the result of > a nested sed invocation using process-banner.sed (which of course would > need adjusting some)? (Maybe the double quotes would need omitting here, > for process-banner.sed to uniformly apply them.) While I agree with Jan that this is kind of ugly, it is a unique case and I would prefer an ugly but simple solution than a more complex solution. This would be different if we were talking about a widespread pattern, in which case the price for complexity would be worth it. My 2 cents in this case are in favor of the simplest approach. I would ack this patch.
Re: [PATCH] xen: Give compile.h header guards
On 2025-05-19 21:10, Jan Beulich wrote: On 19.05.2025 15:52, Andrew Cooper wrote: Signed-off-by: Andrew Cooper Is this to please Misra in some way? Directive 4.10: "Precautions shall be taken in order to prevent the contents of a header file being included more than once". One approach is to special-case this file, but Andrew suggested this approach which addresses the issue directly. --- a/xen/include/xen/compile.h.in +++ b/xen/include/xen/compile.h.in @@ -1,3 +1,6 @@ +#ifndef XEN_COMPILE_H +#define XEN_COMPILE_H + #define XEN_COMPILE_DATE "@@date@@" #define XEN_COMPILE_TIME "@@time@@" #define XEN_COMPILE_BY "@@whoami@@" --- a/xen/tools/process-banner.sed +++ b/xen/tools/process-banner.sed @@ -12,3 +12,8 @@ s_(.*)_"\1\\n"_ # Trailing \ on all but the final line. $!s_$_ \\_ + +# Append closing header guard +$a\ +\ +#endif /* XEN_COMPILE_H */ This split of #ifndef and #endif is ugly. Can't we switch to something more conventional, like #define XEN_BANNER "@@banner@@" with the first sed invocation then replacing this by the result of a nested sed invocation using process-banner.sed (which of course would need adjusting some)? (Maybe the double quotes would need omitting here, for process-banner.sed to uniformly apply them.) Jan -- Nicola Vetrini, B.Sc. Software Engineer BUGSENG (https://bugseng.com) LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
Re: [PATCH] xen: Give compile.h header guards
On 19.05.2025 15:52, Andrew Cooper wrote: > Signed-off-by: Andrew Cooper Is this to please Misra in some way? > --- a/xen/include/xen/compile.h.in > +++ b/xen/include/xen/compile.h.in > @@ -1,3 +1,6 @@ > +#ifndef XEN_COMPILE_H > +#define XEN_COMPILE_H > + > #define XEN_COMPILE_DATE "@@date@@" > #define XEN_COMPILE_TIME "@@time@@" > #define XEN_COMPILE_BY "@@whoami@@" > --- a/xen/tools/process-banner.sed > +++ b/xen/tools/process-banner.sed > @@ -12,3 +12,8 @@ s_(.*)_"\1\\n"_ > > # Trailing \ on all but the final line. > $!s_$_ \\_ > + > +# Append closing header guard > +$a\ > +\ > +#endif /* XEN_COMPILE_H */ This split of #ifndef and #endif is ugly. Can't we switch to something more conventional, like #define XEN_BANNER "@@banner@@" with the first sed invocation then replacing this by the result of a nested sed invocation using process-banner.sed (which of course would need adjusting some)? (Maybe the double quotes would need omitting here, for process-banner.sed to uniformly apply them.) Jan
