Re: [XEN PATCH 1/3] x86/emul: define pseudo keyword fallthrough

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 6 Nov 2024, Jan Beulich wrote:
> On 06.11.2024 10:04, Federico Serafini wrote:
> > The pseudo keyword fallthrough shall be used to make explicit the
> > fallthrough intention at the end of a case statement (doing this
> > through comments is deprecated).
> > 
> > Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> > ---
> >  xen/arch/x86/x86_emulate/x86_emulate.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> 
> When you had asked my privately on Matrix, I specifically said: "Adding
> the pseudo-keyword to x86-emulate.h (not x86_emulate.h) is probably best,
> unless problems with that approach turn up." Even if identical re-
> definitions are deemed fine, I for one consider such bad practice. Yet
> by playing with this file (and outside of any relevant #ifdef) means
> there will be such a re-definition when building Xen itself.
> 
> In fact the patch subject should also already clarify that the auxiliary
> definition is only needed for the test and fuzzing harnesses.

Hi Jan, I don't understand this comment.

You say "playing with this file (and outside of any relevant #ifdef)"
but actually the changes are within the #ifndef
__X86_EMULATE_H__/#endif. What do you mean?

You say "Adding the pseudo-keyword to x86-emulate.h (not x86_emulate.h)
is probably best". I am not very familiar with x86-isms but the only
x86-emulate.h I can find is ./tools/tests/x86_emulator/x86-emulate.h
which is not a header that would help define anything for the Xen build?

I am not understanding your suggestions. From what I can see this patch
looks OK?


> > --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> > @@ -23,6 +23,16 @@
> >  # error Unknown compilation width
> >  #endif
> >  
> > +/*
> > + * Pseudo keyword 'fallthrough' to make explicit the fallthrough intention 
> > at
> > + * the end of a case statement.
> > + */
> > +#if (!defined(__clang__) && (__GNUC__ >= 7))
> 
> I realize xen/compiler.h has it like that, but may I ask that you omit
> the meaningless outer pair of parentheses?
> 
> Jan
>