Changes macro usage to avoid shadowing a variable.
PATCH 4/4 Changes macro usage to avoid shadowing a variable.
Changes the usage of DEF_FIELD_ADDR in this function to create a reference and operate over it using an aux variable. It also changes the loop logic used to find duplicates, to avoid creating another variable.
--- scripts/mod/file2alias.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
scripts/mod/file2alias.c +8 -6
@@ -641,25 +641,27 @@ static void do_pnp_card_entries(void *symval, unsigned long size,unsigned int i;device_id_check (mod->name,"pnp" , size, id_size, symval); +DEF_FIELD_ADDR (symval, pnp_card_device_id, devs); +typeof (devs) devs_last;for (i =0 ; i < count; i++) {unsigned int j; -DEF_FIELD_ADDR (symval + i*id_size, pnp_card_device_id, devs); + devs_last = devs + i * id_size;for (j =0 ; j <PNP_MAX_DEVICES ; j++) { -const char *id = (char *)(*devs)[j].id; -int i2, j2; +const char *id = (char *)(*devs_last)[j].id; +int j2;int dup =0 ;if (!id[0 ])break ;/* find duplicate, already added value */ -for (i2 =0 ; i2 < i && !dup; i2++) { -DEF_FIELD_ADDR (symval + i2*id_size, pnp_card_device_id, devs); +while ((devs_last -= id_size) >= devs) {for (j2 =0 ; j2 <PNP_MAX_DEVICES ; j2++) { -const char *id2 = (char *)(*devs)[j2].id; +const char *id2 = + (char *)(*devs_last)[j2].id;if (!id2[0 ])break ;
Signature · 1 line
2.19.1
Hi Leonardo,
Thanks for the patch, just some small comments below.
Quoted 30 lines · Leonardo Brás
Changes the usage of
DEF_FIELD_ADDRin this function to create a reference and operate over it using an aux variable. It also changes the loop logic used to find duplicates, to avoid creating another variable.Signed-off-by Leonardo Brás <[email protected]>--- scripts/mod/file2alias.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)scripts/mod/file2alias.c +4
-3@@ -641,25 +641,27 @@ static void do_pnp_card_entries(void *symval, unsigned long size,unsigned int i;device_id_check (mod->name,"pnp" , size, id_size, symval); +DEF_FIELD_ADDR (symval, pnp_card_device_id, devs); +typeof (devs) devs_last;for (i =0 ; i < count; i++) {unsigned int j; -DEF_FIELD_ADDR (symval + i*id_size, pnp_card_device_id, devs); + devs_last = devs + i * id_size;for (j =0 ; j <PNP_MAX_DEVICES ; j++) { -const char *id = (char *)(*devs)[j].id; -int i2, j2; +const char *id = (char *)(*devs_last)[j].id;
scripts/mod/file2alias.c static void do_pnp_card_entries(void *symval, unsigned long size, · line 660
Quoted 10 lines
scripts/mod/file2alias.c +2
-2+int j2;int dup =0 ;if (!id[0 ])break ;/* find duplicate, already added value */ -for (i2 =0 ; i2 < i && !dup; i2++) { -DEF_FIELD_ADDR (symval + i2*id_size, pnp_card_device_id, devs); +while ((devs_last -= id_size) >= devs) {
You forgot to consider the dup variable.
Also you inverted the order of this loop, I am not sure this is important (I just took a quick look) but you need to be careful.
This is also hard to read, I would define another variant macro where you can set any name of the variable, e.g.
#define DEF_FIELD_ADDR_VAR(m, devid, f, var) \ typeof(((struct devid *)0)->f) *var = ((m) + OFF_##devid##_##f)
In this way you don't need to change the logic, just the name of the variable.
scripts/mod/file2alias.c static void do_pnp_card_entries(void *symval, unsigned long size,
Quoted 9 lines
scripts/mod/file2alias.c +2
-1for (j2 =0 ; j2 <PNP_MAX_DEVICES ; j2++) { -const char *id2 = (char *)(*devs)[j2].id; +const char *id2 = + (char *)(*devs_last)[j2].id;if (!id2[0 ])break ;
Regards,
Helen
Hello Helen,
Quoted 42 lines · Helen Koike
Hi Leonardo,
Thanks for the patch, just some small comments below.
Quoted 36 lines · Leonardo Brás
Changes the usage of
DEF_FIELD_ADDRin this function to create a reference and operate over it using an aux variable. It also changes the loop logic used to find duplicates, to avoid creating another variable.Signed-off-by Leonardo Brás <[email protected]>--- scripts/mod/file2alias.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)scripts/mod/file2alias.c +5
-3@@ -641,25 +641,27 @@ static void do_pnp_card_entries(void *symval, unsigned long size,unsigned int i;device_id_check (mod->name,"pnp" , size, id_size, symval); +DEF_FIELD_ADDR (symval, pnp_card_device_id, devs); +typeof (devs) devs_last;for (i =0 ; i < count; i++) {unsigned int j; -DEF_FIELD_ADDR (symval + i*id_size, pnp_card_device_id, devs); + devs_last = devs + i * id_size;for (j =0 ; j <PNP_MAX_DEVICES ; j++) { -const char *id = (char *)(*devs)[j].id; -int i2, j2; +const char *id = (char *)(*devs_last)[j].id; +int j2;int dup =0 ;if (!id[0 ])break ;
Quoted 6 lines
scripts/mod/file2alias.c static void do_pnp_card_entries(void *symval, unsigned long size, · line 660
Quoted 4 lines
scripts/mod/file2alias.c +1
-2/* find duplicate, already added value */ -for (i2 =0 ; i2 < i && !dup; i2++) { -DEF_FIELD_ADDR (symval + i2*id_size, pnp_card_device_id, devs); +while ((devs_last -= id_size) >= devs) {You forgot to consider the dup variable.
Sorry, I did not get it. Could you explain it?
Quoted 2 lines
Also you inverted the order of this loop, I am not sure this is important (I just took a quick look) but you need to be careful.
It seems to be looking for a duplicate. I don't think inverting will break anything.
But I will give a better look anyway.
Quoted 2 lines
This is also hard to read,
Humm, IMO it looks easier to read this way. But ok, I can change it back.
Quoted 9 lines
I would define another variant macro where
you can set any name of the variable, e.g.#define DEF_FIELD_ADDR_VAR(m, devid, f, var) \ typeof(((struct devid *)0)->f) *var = ((m) + OFF_##devid##_##f)In this way you don't need to change the logic, just the name of the variable.
Good suggestion. Is it ok to create a macro for using only once? It looks like its not worth the trouble.
Quoted 6 lines
Quoted 6 lines
scripts/mod/file2alias.c +2
-1for (j2 =0 ; j2 <PNP_MAX_DEVICES ; j2++) { -const char *id2 = (char *)(*devs)[j2].id; +const char *id2 = + (char *)(*devs_last)[j2].id;
Quoted 6 lines
scripts/mod/file2alias.c static void do_pnp_card_entries(void *symval, unsigned long size,
Quoted 3 lines
scripts/mod/file2alias.c +0
-0if (!id2[0 ])break ;Regards,
Helen
Thank you for reviewing,
Leonardo Bras