| Message ID | 20211130181238.5501-1-ayankuma@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] bitops: Fix incorrect value in comment | expand |
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>
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 >
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,
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
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,
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
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/
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))))
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(-)