bitops: Fix incorrect value in comment

[PATCH v2] bitops: Fix incorrect value in comment

Posted by Ayan Kumar Halder 2 years, 4 months ago

GENMASK(30, 21) should be 0x7fe00000. Fixed this in the comment
in bitops.h.

Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
---
Changelog :-
v2 :- 1. Replaced the word "vector" with "value" in comment.
2. Changed 0x07fe00000 to 0x7fe00000.
3. Updated the commit message to make it meaningful.
(All suggested by Jan Beulich)

 xen/include/xen/bitops.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
index a64595f68e..dad4b5aa1e 100644
--- 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.
  */
 #define GENMASK(h, l) \
     (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
-- 
2.17.1

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

Posted by Jan Beulich 2 years, 4 months ago

On 30.11.2021 19:12, Ayan Kumar Halder wrote:
> GENMASK(30, 21) should be 0x7fe00000. Fixed this in the comment
> in bitops.h.
> 
> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

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

Posted by Bertrand Marquis 2 years, 4 months ago

Hi Ayan,

> On 30 Nov 2021, at 18:12, Ayan Kumar Halder <ayan.kumar.halder@xilinx.com> wrote:
> 
> GENMASK(30, 21) should be 0x7fe00000. Fixed this in the comment
> in bitops.h.
> 
> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> Changelog :-
> v2 :- 1. Replaced the word "vector" with "value" in comment.
> 2. Changed 0x07fe00000 to 0x7fe00000.
> 3. Updated the commit message to make it meaningful.
> (All suggested by Jan Beulich)
> 
> xen/include/xen/bitops.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
> index a64595f68e..dad4b5aa1e 100644
> --- 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.
>  */
> #define GENMASK(h, l) \
>     (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
> -- 
> 2.17.1
>

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

Posted by Julien Grall 2 years, 4 months ago

Hi,

On 30/11/2021 18:12, Ayan Kumar Halder wrote:
> GENMASK(30, 21) should be 0x7fe00000. Fixed this in the comment
> in bitops.h.

I am afraid this commit message is incomplete. You say you just 
corrected the bitmask returned but...

> 
> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
> ---
> Changelog :-
> v2 :- 1. Replaced the word "vector" with "value" in comment.
> 2. Changed 0x07fe00000 to 0x7fe00000.
> 3. Updated the commit message to make it meaningful.
> (All suggested by Jan Beulich)
> 
>   xen/include/xen/bitops.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
> index a64595f68e..dad4b5aa1e 100644
> --- 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.

Cheers,

-- 
Julien Grall

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

Posted by Jan Beulich 2 years, 4 months ago

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?

I also think the commit message is quite fine as is.

Jan

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

Posted by Julien Grall 2 years, 4 months ago

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.

Cheers,

-- 
Julien Grall

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

Posted by Andrew Cooper 2 years, 4 months ago

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

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

Posted by Julien Grall 2 years, 4 months ago

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@suse.com/

-- 
Julien Grall