Re: [Xen-devel] [PATCH RFC for-4.13 07/10] xen/arm: Allow insn.h to be called from assembly

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 21 Oct 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 10/1/19 10:00 PM, Stefano Stabellini wrote:
> > On Thu, 26 Sep 2019, Julien Grall wrote:
> > > A follow-up patch will require to include insn.h from assembly code. So
> > > wee need to protect any C-specific definition to avoid compilation
> >    ^ we                               ^ definitions
> > 
> > > error when used in assembly code.
> >    ^ errors
> > 
> > 
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > > ---
> > >   xen/include/asm-arm/insn.h | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/xen/include/asm-arm/insn.h b/xen/include/asm-arm/insn.h
> > > index 19277212e1..00391f83f9 100644
> > > --- a/xen/include/asm-arm/insn.h
> > > +++ b/xen/include/asm-arm/insn.h
> > > @@ -1,8 +1,14 @@
> > >   #ifndef __ARCH_ARM_INSN
> > >   #define __ARCH_ARM_INSN
> > >   +#ifndef __ASSEMBLY__
> > > +
> > >   #include <xen/types.h>
> > >   +/*
> > > + * At the moment, arch-specific headers contain only definition for C
> > > + * code.
> > > + */
> > 
> > Please remove "At the moment, " because in-code comment should always
> > reflect the latest state of the codebase.
> 
> Well, we do have a lot of "Currently"/"At the moment"/"For now" in the code.
> Some of my patches went in recently with such wording, so why this particular
> one is a problem?

Yeah, I realize that a lot of our conventions are still tribal knowledge
(i.e. not written down anywhere.)

Let me explain my take on this. When one writes "At the
moment"/"Currently"/"For now" he/she implies that the code is just
temporary, or at least sub-optimal and something should be down about
it to improve. Maybe not immediately, but it would be nice to have.

I think it is OK to write something like "At the moment," to express
that meaning. That's OK.

In this specific case, the reason why I wrote that reply is that when I
read the in-code comment, it jarred for it ("jarred" as in something was
off, out of place.) And I think the reason is that the code looked OK,
and I didn't get what you think we should "improve" in the code after
this patch is applied.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel