[Outreachy Kernel] [PATCH] staging: greybus: Constify local struct gb_loopback_stats

[Outreachy Kernel] [PATCH] staging: greybus: Constify local struct gb_loopback_stats

[Rehas Sachdeva]

The local declaration "struct gb_loopback_stats reset" is not modified
throughout its scope and hence can be made const. Done using Coccinelle.

Signed-off-by: Rehas Sachdeva <aquannie@gmail.com>
---
 drivers/staging/greybus/loopback.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
index 7882306..fb5a068 100644
--- a/drivers/staging/greybus/loopback.c
+++ b/drivers/staging/greybus/loopback.c
@@ -837,7 +837,7 @@ static int gb_loopback_request_handler(struct gb_operation *operation)
 
 static void gb_loopback_reset_stats(struct gb_loopback *gb)
 {
-	struct gb_loopback_stats reset = {
+	const struct gb_loopback_stats reset = {
 		.min = U32_MAX,
 	};
 
-- 
2.7.4

Re: [Outreachy kernel] [Outreachy Kernel] [PATCH] staging: greybus: Constify local struct gb_loopback_stats

[Julia Lawall]

On Mon, 3 Oct 2016, Rehas Sachdeva wrote:

> The local declaration "struct gb_loopback_stats reset" is not modified
> throughout its scope and hence can be made const. Done using Coccinelle.
>
> Signed-off-by: Rehas Sachdeva <aquannie@gmail.com>
> ---
>  drivers/staging/greybus/loopback.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> index 7882306..fb5a068 100644
> --- a/drivers/staging/greybus/loopback.c
> +++ b/drivers/staging/greybus/loopback.c
> @@ -837,7 +837,7 @@ static int gb_loopback_request_handler(struct gb_operation *operation)
>
>  static void gb_loopback_reset_stats(struct gb_loopback *gb)
>  {
> -	struct gb_loopback_stats reset = {
> +	const struct gb_loopback_stats reset = {

Should it also be static?  I recall this comment being made in another
case, but I don't know what the tradeoffs are.

julia

>  		.min = U32_MAX,
>  	};
>
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20161002192224.GA5295%40toblerone.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [Outreachy Kernel] [PATCH] staging: greybus: Constify local struct gb_loopback_stats

[Greg Kroah-Hartman]

On Sun, Oct 02, 2016 at 09:36:39PM +0200, Julia Lawall wrote:
> 
> 
> On Mon, 3 Oct 2016, Rehas Sachdeva wrote:
> 
> > The local declaration "struct gb_loopback_stats reset" is not modified
> > throughout its scope and hence can be made const. Done using Coccinelle.
> >
> > Signed-off-by: Rehas Sachdeva <aquannie@gmail.com>
> > ---
> >  drivers/staging/greybus/loopback.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> > index 7882306..fb5a068 100644
> > --- a/drivers/staging/greybus/loopback.c
> > +++ b/drivers/staging/greybus/loopback.c
> > @@ -837,7 +837,7 @@ static int gb_loopback_request_handler(struct gb_operation *operation)
> >
> >  static void gb_loopback_reset_stats(struct gb_loopback *gb)
> >  {
> > -	struct gb_loopback_stats reset = {
> > +	const struct gb_loopback_stats reset = {
> 
> Should it also be static?  I recall this comment being made in another
> case, but I don't know what the tradeoffs are.

I'll let Bryan answer that one, he should know...

Bryan?

Re: [Outreachy kernel] [Outreachy Kernel] [PATCH] staging: greybus: Constify local struct gb_loopback_stats

[Rehas Sachdeva]

On Mon, Oct 03, 2016 at 11:16:03AM +0100, Bryan O'Donoghue wrote:
> On Sun, 2016-10-02 at 21:36 +0200, Julia Lawall wrote:
> > 
> > On Mon, 3 Oct 2016, Rehas Sachdeva wrote:
> > 
> > > The local declaration "struct gb_loopback_stats reset" is not
> > modified
> > > throughout its scope and hence can be made const. Done using
> > Coccinelle.
> > >
> > > Signed-off-by: Rehas Sachdeva <aquannie@gmail.com>
> > > ---
> > >� drivers/staging/greybus/loopback.c | 2 +-
> > >� 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/greybus/loopback.c
> > b/drivers/staging/greybus/loopback.c
> > > index 7882306..fb5a068 100644
> > > --- a/drivers/staging/greybus/loopback.c
> > > +++ b/drivers/staging/greybus/loopback.c
> > > @@ -837,7 +837,7 @@ static int gb_loopback_request_handler(struct
> > gb_operation *operation)
> > >
> > >� static void gb_loopback_reset_stats(struct gb_loopback *gb)
> > >� {
> > > -�����struct gb_loopback_stats reset = {
> > > +�����const struct gb_loopback_stats reset = {
> > 
> > Should it also be static?� I recall this comment being made in
> > another
> > case, but I don't know what the tradeoffs are.
> 

Yes it can be static too, since we are not updating it, so we don't care
if its value is retained in subsequent function calls. I agree with Bryan that
we might as well make all locally scoped const variables static too.

Making it static decreases the size of text segment, without affecting any
other segment. I don't know if its particularly useful.

Rehas

> const yes agree - good catch.
> 
> static ... can't see a reason why TBH. Making this variable static we
> might as well declare all locally scoped variables static.
> 
> ---
> bod

Re: [Outreachy kernel] [Outreachy Kernel] [PATCH] staging: greybus: Constify local struct gb_loopback_stats

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4806 bytes --]



On Mon, 3 Oct 2016, Bryan O'Donoghue wrote:

> On Mon, 2016-10-03 at 16:14 +0530, Rehas Sachdeva wrote:
> > On Mon, Oct 03, 2016 at 11:16:03AM +0100, Bryan O'Donoghue wrote:
> > >
> > > On Sun, 2016-10-02 at 21:36 +0200, Julia Lawall wrote:
> > > >
> > > >
> > > > On Mon, 3 Oct 2016, Rehas Sachdeva wrote:
> > > >
> > > > >
> > > > > The local declaration "struct gb_loopback_stats reset" is not
> > > > modified
> > > > >
> > > > > throughout its scope and hence can be made const. Done using
> > > > Coccinelle.
> > > > >
> > > > >
> > > > > Signed-off-by: Rehas Sachdeva <aquannie@gmail.com>
> > > > > ---
> > > > >   drivers/staging/greybus/loopback.c | 2 +-
> > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/staging/greybus/loopback.c
> > > > b/drivers/staging/greybus/loopback.c
> > > > >
> > > > > index 7882306..fb5a068 100644
> > > > > --- a/drivers/staging/greybus/loopback.c
> > > > > +++ b/drivers/staging/greybus/loopback.c
> > > > > @@ -837,7 +837,7 @@ static int
> > > > > gb_loopback_request_handler(struct
> > > > gb_operation *operation)
> > > > >
> > > > >
> > > > >   static void gb_loopback_reset_stats(struct gb_loopback *gb)
> > > > >   {
> > > > > -     struct gb_loopback_stats reset = {
> > > > > +     const struct gb_loopback_stats reset = {
> > > > Should it also be static?  I recall this comment being made in
> > > > another
> > > > case, but I don't know what the tradeoffs are.
> > Yes it can be static too, since we are not updating it, so we don't
> > care
> > if its value is retained in subsequent function calls. I agree with
> > Bryan that
> > we might as well make all locally scoped const variables static too.
> >
> > Making it static decreases the size of text segment, without
> > affecting any
> > other segment. I don't know if its particularly useful.
> >
> > Rehas
>
> Hmm..
>
> Looking at the resulting assembler on x86 anyway - I do kind of agree
> that static makes sense. For this compound type the # of operations we
> end up executing is a fair bit less for static (v) non-static will very
> little penalty in the .text section.
>
> ### static ###
>
> somefunction:
> .LFB0:
>         .cfi_startproc
>         pushq   %rbp
>         .cfi_def_cfa_offset 16
>         .cfi_offset 6, -16
>         movq    %rsp, %rbp
>         .cfi_def_cfa_register 6
>         subq    $16, %rsp
>         movq    %rdi, -8(%rbp)
>         movq    -8(%rbp), %rax
>         movl    $80, %edx
>         movl    $assign.2474, %esi
>         movq    %rax, %rdi 
>         call    memcpy
>         nop
>         leave   
>         .cfi_def_cfa 7, 8
>         ret
>         .cfi_endproc 
>
> ### non-static ###
>
> somefunction:
> .LFB0:
>         .cfi_startproc
>         pushq   %rbp
>         .cfi_def_cfa_offset 16
>         .cfi_offset 6, -16
>         movq    %rsp, %rbp
>         .cfi_def_cfa_register 6
>         subq    $112, %rsp
>         movq    %rdi, -104(%rbp)
>         movq    %fs:40, %rax
>         movq    %rax, -8(%rbp)
>         xorl    %eax, %eax
>         leaq    -96(%rbp), %rdx
>         movl    $0, %eax
>         movl    $10, %ecx
>         movq    %rdx, %rdi
>         rep stosq
>         movl    $10, -96(%rbp)
>         movl    $11, -92(%rbp)
>         leaq    -96(%rbp), %rcx
>         movq    -104(%rbp), %rax
>         movl    $80, %edx
>         movq    %rcx, %rsi
>         movq    %rax, %rdi
>         call    memcpy
>         nop
>         movq    -8(%rbp), %rax
>         xorq    %fs:40, %rax
>         je      .L2
>         call    __stack_chk_fail
>
> #### test program ####
> #include <stdio.h>
> #include <string.h>
>
> struct a {
> 	int a;
> 	int b;
> 	int c;
> 	int d;
> 	int e;
> 	int f;
> 	int g;
> 	int h;
> 	int i;
> 	int j;
> 	int k;
> 	int l;
> 	int m;
> 	int n;
> 	int o;
> 	int p;
> 	int q;
> 	int r;
> 	int s;
> 	int t;
> };
>
> void somefunction(struct a *assign_me)
> {
> 	const struct a assign = {
> 		.a = 10,
> 		.b = 11,
> 	};
>
> 	memcpy(assign_me, &assign, sizeof(assign));
> }
>
> int main(int argc, char *argv[])
> {
> 	struct a assign;
>
> 	somefunction(&assign);
> 	printf("values of assignment are %d and %d\n", assign.a,
> assign.b);
>
> 	return 0;
> }
>
>
> So actually I'd say static is a good call

Thanks for looking into it.

julia