Re: [PATCH v2] bitops: Fix incorrect value in comment

[Julien Grall <julien@xxxxxxx>]

Hi Andrew,

On 01/12/2021 21:38, Andrew Cooper wrote:
> On 01/12/2021 09:56, Julien Grall wrote:
> > Hi,
> >
> > On 01/12/2021 09:38, Jan Beulich wrote:
> > > On 01.12.2021 10:33, Julien Grall wrote:
> > > > On 30/11/2021 18:12, Ayan Kumar Halder wrote:
> > > > > --- a/xen/include/xen/bitops.h
> > > > > +++ b/xen/include/xen/bitops.h
> > > > > @@ -5,7 +5,7 @@
> > > > >     /*
> > > > >      * Create a contiguous bitmask starting at bit position @l and
> > > > > ending at
> > > > >      * position @h. For example
> > > > > - * GENMASK(30, 21) gives us the 32bit vector 0x01fe00000.
> > > > > + * GENMASK(30, 21) gives us the 32bit value 0x7fe00000.
> > > > ... there are two extra changes here:
> > > >      1) The bitmask is now described with 8-characters (rather than 9)
> > > >      2) 'vector' is replaced with 'value'
> > > >
> > > > The former makes sense to me, but it is not clear to me why the latter
> > > > should be changed.
> > > Would you mind explaining to me in which way you see "vector" accurately
> > > describe the entity talked about?
> > This can be seen as a vector of bit. I can see why people may think
> > otherwise. However... if you think it doesn't describe it accurately,
> > then I think this ought to be changed in Linux first (where the code
> > and comment comes from).
> >
> > > I also think the commit message is quite fine as is.
> > IMHO, this is similar to when one do coding style change in a patch.
> > They are unrelated but would be acceptable so long they are explained
> > in the commit message.
> >
> > What I request is something like:
> >
> > "GENMASK(30, 21) should be 0x7fe00000 and only use 8-characters (it is
> > a 32-bit comment). Fixed this in the comment.
> >
> > Take the opportunity to replace 'vector' with 'value' because..."
> >
> > This is simple enough and clarify what is the intent of the patch.
> This is an unreasonable quantity of bikeshedding. 
I didn't realize that two emails were considered bikeshedding.
I actually provided and worked towards a solution rather than
unhelpfully saying just no.

> It's just a comment,
> and a commit message of "fix the comment" is perfectly fine.
> Furthermore, the intent of the text is clear.
>
> However, "32bit $WHATEVER" is also wrong because GENMASK() yields a
> unsigned long constant.  Importantly, not 32 bits in an ARM64 build.
>
>
> This trivial patch has lingered far too long.  I have committed it,
> along with an adjustment.  Further bikeshedding will be redirected to
> /dev/null.
It is an interesting approach. I could have committed this patch
after updating the commit message like you did ;).

But, so far, I have refrained from blatantly ignoring comments and going
ahead with committing ([1] is an example where this could be used)
because I think we should try to have a consensus first.

Anyway, I am happy to accept that two maintainers have an opposite view 
from me and go with the tide. That said, there are probably better a way 
to express your view...
Cheers,

[1] 
https://lore.kernel.org/xen-devel/062bcbd3-420e-e1c0-3aa0-0dfb229e6ae9@xxxxxxxx/
--
Julien Grall