staging: greybus: Constify local struct gb_loopback_stats

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 <[email protected]>

---
 drivers/staging/greybus/loopback.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/greybus/loopback.c +0 -0

index 7882306..fb5a068 100644

drivers/staging/greybus/loopback.c +0 -1

 		.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 [email protected]. To post to this group, send email to [email protected].
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.

Quoted 14 lines · 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 <[email protected]>

---
 drivers/staging/greybus/loopback.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/greybus/loopback.c +0 -0

index 7882306..fb5a068 100644
@@ -837,7 +837,7 @@ static int gb_loopback_request_handler(struct gb_operation *operation)
 

drivers/staging/greybus/loopback.c static int gb_loopback_request_handler(struct gb_operation *operation) · line 840

Quoted 4 lines

drivers/staging/greybus/loopback.c +1 -1

 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.

Quoted 21 lines · Julia Lawall

Quoted 1 line · Rehas Sachdeva

The local declaration "struct gb_loopback_stats reset" is not

modified

Quoted 1 line

throughout its scope and hence can be made const. Done using

Coccinelle.

Review

Quoted 7 lines

Signed-off-by Rehas Sachdeva <[email protected]>

---
� drivers/staging/greybus/loopback.c | 2 +-
� 1 file changed, 1 insertion(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/staging/greybus/loopback.c

drivers/staging/greybus/loopback.c static int gb_loopback_request_handler(struct

Quoted 4 lines

index 7882306..fb5a068 100644

drivers/staging/greybus/loopback.c +0 -0

@@ -837,7 +837,7 @@ static int gb_loopback_request_handler(struct

gb_operation *operation)

Quoted 1 line

Quoted 8 lines

Quoted 4 lines

� 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.

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

Quoted 44 lines · Rehas Sachdeva

Quoted 30 lines · Bryan O'Donoghue

Quoted 28 lines · Julia Lawall

Quoted 2 lines · Rehas Sachdeva

The local declaration "struct gb_loopback_stats reset" is not

modified

Quoted 2 lines

throughout its scope and hence can be made const. Done using

Coccinelle.

Review

Quoted 8 lines

Signed-off-by Rehas Sachdeva <[email protected]>

---
  drivers/staging/greybus/loopback.c | 2 +-

  1 file changed, 1 insertion(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/staging/greybus/loopback.c

Quoted 6 lines

index 7882306..fb5a068 100644

drivers/staging/greybus/loopback.c +0 -0

@@ -837,7 +837,7 @@ static int

gb_loopback_request_handler(struct

gb_operation *operation)

Quoted 2 lines

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

  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.

Quoted 5 lines

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