| Message ID | 20181017000943.GA21382@WindFlash (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Adds -Wshadow=local on KBUILD_HOSTCFLAGS | expand |
Hi Leonardo, Thanks for the patch, just some small comments below. On 10/16/18 9:09 PM, Leonardo Brás wrote: > 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. > > Signed-off-by: Leonardo Brás <leobras.c@gmail.com> > --- > scripts/mod/file2alias.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c > index 7be43697ff84..9ea1db2aefdb 100644 > --- a/scripts/mod/file2alias.c > +++ b/scripts/mod/file2alias.c > @@ -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) { 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. > > 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; > Regards, Helen
Hello Helen, On Wed, Oct 17, 2018 at 1:18 AM Helen Koike <helen@koikeco.de> wrote: > > Hi Leonardo, > > Thanks for the patch, just some small comments below. > > On 10/16/18 9:09 PM, Leonardo Brás wrote: > > 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. > > > > Signed-off-by: Leonardo Brás <leobras.c@gmail.com> > > --- > > scripts/mod/file2alias.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c > > index 7be43697ff84..9ea1db2aefdb 100644 > > --- a/scripts/mod/file2alias.c > > +++ b/scripts/mod/file2alias.c > > @@ -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) { > > You forgot to consider the dup variable. Sorry, I did not get it. Could you explain it? > 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. > > This is also hard to read, Humm, IMO it looks easier to read this way. But ok, I can change it back. > 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. > > > > 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; > > > > Regards, > Helen Thank you for reviewing, Leonardo Bras
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c index 7be43697ff84..9ea1db2aefdb 100644 --- a/scripts/mod/file2alias.c +++ b/scripts/mod/file2alias.c @@ -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;
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. Signed-off-by: Leonardo Brás <leobras.c@gmail.com> --- scripts/mod/file2alias.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)