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

[Andrew Cooper <amc96@xxxxxxxx>]

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.  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.

~Andrew