[PATCH] arch/x86/boot: sizeof macro

[PATCH] arch/x86/boot: sizeof macro

[Saidgani Musaev]

---
 arch/x86/boot/memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/boot/memory.c b/arch/x86/boot/memory.c
index d9c28c8..7df2b28 100644
--- a/arch/x86/boot/memory.c
+++ b/arch/x86/boot/memory.c
@@ -26,7 +26,7 @@ static int detect_memory_e820(void)
 
 	initregs(&ireg);
 	ireg.ax  = 0xe820;
-	ireg.cx  = sizeof buf;
+	ireg.cx  = sizeof(buf);
 	ireg.edx = SMAP;
 	ireg.di  = (size_t)&buf;
 
-- 
2.9.5

Re: [PATCH] arch/x86/boot: sizeof macro

[Andy Shevchenko]

On Fri, Dec 29, 2017 at 8:30 PM, Saidgani Musaev <cpu808694@gmail.com> wrote:

No changelog, no explanation why...
No SoB tag...

> -       ireg.cx  = sizeof buf;
> +       ireg.cx  = sizeof(buf);

sizeof is operator, not a function.
So, what are you trying to achieve?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] arch/x86/boot: sizeof macro

[hacku]

Coding style for sizeof . If you check this file with checkpatch.pl you 
will see warning on this line. Yes, sorry I didn't mark this change as a 
coding style issue.

best regards,
Saidgani.

On 12/29/2017 07:41 PM, Andy Shevchenko wrote:
> On Fri, Dec 29, 2017 at 8:30 PM, Saidgani Musaev <cpu808694@gmail.com> wrote:
>
> No changelog, no explanation why...
> No SoB tag...
>
>> -       ireg.cx  = sizeof buf;
>> +       ireg.cx  = sizeof(buf);
> sizeof is operator, not a function.
> So, what are you trying to achieve?
>

Re: [PATCH] arch/x86/boot: sizeof macro

[Ingo Molnar]

* Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Dec 29, 2017 at 8:30 PM, Saidgani Musaev <cpu808694@gmail.com> wrote:
> 
> No changelog, no explanation why...
> No SoB tag...
> 
> > -       ireg.cx  = sizeof buf;
> > +       ireg.cx  = sizeof(buf);
> 
> sizeof is operator, not a function.
> So, what are you trying to achieve?

That's true, but in the kernel coding style we use it as a function:

  triton:~/tip> git grep 'sizeof(' | wc -l
  116570

  triton:~/tip> git grep 'sizeof [:alnum:]' | wc -l
  177

Thanks,

	Ingo

Re: [PATCH] arch/x86/boot: sizeof macro

[Andy Shevchenko]

On Fri, Dec 29, 2017 at 9:46 PM, Ingo Molnar <mingo@kernel.org> wrote:
> * Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

>> > -       ireg.cx  = sizeof buf;
>> > +       ireg.cx  = sizeof(buf);
>>
>> sizeof is operator, not a function.
>> So, what are you trying to achieve?
>
> That's true, but in the kernel coding style we use it as a function:

Yeah, the question is do we need to change all those 177 occurrences at all?
Or perhaps it might be an opportunity during some other changes to
certain users...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] arch/x86/boot: sizeof macro

[Joe Perches]

On Sun, 2017-12-31 at 13:48 +0200, Andy Shevchenko wrote:
> On Fri, Dec 29, 2017 at 9:46 PM, Ingo Molnar <mingo@kernel.org> wrote:
> > * Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > -       ireg.cx  = sizeof buf;
> > > > +       ireg.cx  = sizeof(buf);
> > > 
> > > sizeof is operator, not a function.
> > > So, what are you trying to achieve?
> > 
> > That's true, but in the kernel coding style we use it as a function:
> 
> Yeah, the question is do we need to change all those 177 occurrences at all?

There's actually around 900, for ~100:1 ratio

$ git grep -P "\bsizeof\s+\w" -- "*.[ch]" | wc -l
907
$ git grep -P "\bsizeof\s*\(" -- "*.[ch]" | wc -l
117203

About half of these occurrences are in 3 paths
and their actual uses of sizeof without and with
parenthesis are:

drivers/infiniband/	212:3696
drivers/media/		111:5058
drivers/usb/		121:2006

> Or perhaps it might be an opportunity during some other changes to
> certain users...

Perhaps best that way.

Re: [PATCH] arch/x86/boot: sizeof macro

[Ingo Molnar]

* Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Dec 29, 2017 at 9:46 PM, Ingo Molnar <mingo@kernel.org> wrote:
> > * Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> >> > -       ireg.cx  = sizeof buf;
> >> > +       ireg.cx  = sizeof(buf);
> >>
> >> sizeof is operator, not a function.
> >> So, what are you trying to achieve?
> >
> > That's true, but in the kernel coding style we use it as a function:
> 
> Yeah, the question is do we need to change all those 177 occurrences at all?
> Or perhaps it might be an opportunity during some other changes to
> certain users...

We probably don't want to change then, and my point was that 99.8% of the current 
usage is as a 'function':

  triton:~/tip> git grep 'sizeof(' | wc -l
  116570

  triton:~/tip> git grep 'sizeof [:alnum:]' | wc -l
  177

i.e. your patch moves it in the exact wrong direction.

Thanks,

	Ingo

Re: [PATCH] arch/x86/boot: sizeof macro

[H. Peter Anvin]

On 01/06/18 03:50, Ingo Molnar wrote:
> 
> We probably don't want to change then, and my point was that 99.8% of the current 
> usage is as a 'function':
> 
>   triton:~/tip> git grep 'sizeof(' | wc -l
>   116570
> 
>   triton:~/tip> git grep 'sizeof [:alnum:]' | wc -l
>   177
> 
> i.e. your patch moves it in the exact wrong direction.
> 

The bottom test is bogus as it is fairly common to do "sizeof *foo" in
addition to "sizeof foo":

: tazenda 139 ; git grep -P 'sizeof\s*[^\(\s]' | wc -l
2085

... but it certainly doesn't change the fact that the kernel coding
style is quite unambiguous.  However, I don't think it makes any sense
to patch just for the sake of patching.

I personally like the additional visual clarification (backed by
compiler assertion) that the argument is specifically an object, not a
type ("sizeof type" is invalid), but that's not the prevalent use in
kernel code.

	-hpa