Skip to contentSkip to conversation

arch/x86/boot: sizeof macro

PATCH In review Saidgani Musaev 5 participants 8 messages Dec 29, 2017 – Jan 8, 2018 x86

lore
Share & tools
lore

Saidgani Musaev

author
#0 lore raw

PATCH 1/1 arch/x86/boot: sizeof macro

---
 arch/x86/boot/memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/boot/memory.c +1 -1
index d9c28c8..7df2b28 100644
@@ -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;
Signature · 1 line
2.9.5
#1 lore raw

On Fri, Dec 29, 2017 at 8:30 PM, Saidgani Musaev <[email protected]> wrote:

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

arch/x86/boot/memory.c static int detect_memory_e820(void) · line 29

Quoted 2 lines
arch/x86/boot/memory.c +1 -1
-       ireg.cx  = sizeof buf;
+       ireg.cx  = sizeof(buf);

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

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#2 lore raw

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.

Quoted 10 lines · Andy Shevchenko

On Fri, Dec 29, 2017 at 8:30 PM, Saidgani Musaev <[email protected]> wrote:

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

arch/x86/boot/memory.c static int detect_memory_e820(void) · line 29

Quoted 2 lines
arch/x86/boot/memory.c +1 -1
-       ireg.cx  = sizeof buf;
+       ireg.cx  = sizeof(buf);

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

#3 lore raw
In reply to Andy Shevchenko
Quoted 10 lines · Andy Shevchenko

On Fri, Dec 29, 2017 at 8:30 PM, Saidgani Musaev <[email protected]> wrote:

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

arch/x86/boot/memory.c static int detect_memory_e820(void) · line 29

Quoted 2 lines
arch/x86/boot/memory.c +1 -1
-       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

#4 lore raw
Quoted 9 lines · Ingo Molnar
Quoted 5 lines · Andy Shevchenko

arch/x86/boot/memory.c static int detect_memory_e820(void) · line 29

Quoted 2 lines
arch/x86/boot/memory.c +1 -1
-       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...

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#5 lore raw
Quoted 11 lines · Andy Shevchenko
Quoted 8 lines · Ingo Molnar
Quoted 5 lines · Andy Shevchenko

arch/x86/boot/memory.c static int detect_memory_e820(void) · line 29

Quoted 2 lines
arch/x86/boot/memory.c +1 -1
-       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

Quoted 2 lines

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

Perhaps best that way.

#6 lore raw
In reply to Andy Shevchenko
Quoted 6 lines · Andy Shevchenko
Quoted 5 lines · Ingo Molnar
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines
arch/x86/boot/memory.c +1 -1
-       ireg.cx  = sizeof buf;
+       ireg.cx  = sizeof(buf);
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

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

reviewer
#7 lore raw
Quoted 4 lines · Ingo Molnar

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

Quoted 8 lines

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