| Message ID | 1470124474-3489-1-git-send-email-anders.roxell@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 08/02/16 10:54, Anders Roxell wrote: > Signed-off-by: Anders Roxell <anders.roxell@linaro.org> > --- > helper/hashtable.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/helper/hashtable.c b/helper/hashtable.c > index 8bb1ae5..f17b80f 100644 > --- a/helper/hashtable.c > +++ b/helper/hashtable.c > @@ -164,7 +164,7 @@ odph_table_t odph_hash_table_lookup(const char *name) > * This hash algorithm is the most simple one, so we choose it as an DEMO > * User can use any other algorithm, like CRC... > */ > -uint16_t odp_key_hash(void *key, uint32_t key_size) > +static uint16_t odp_key_hash(void *key, uint32_t key_size) odph_ > { > register uint32_t hash = 0; > uint32_t idx = (key_size == 0 ? 1 : key_size); > @@ -181,7 +181,7 @@ uint16_t odp_key_hash(void *key, uint32_t key_size) > /** > * Get an available node from pool > */ > -odph_hash_node *odp_hashnode_take(odph_table_t table) > +static odph_hash_node *odp_hashnode_take(odph_table_t table) > { > odph_hash_table_imp *tbl = (odph_hash_table_imp *)table; > uint32_t idx; > @@ -208,7 +208,7 @@ odph_hash_node *odp_hashnode_take(odph_table_t table) > /** > * Release an node to the pool > */ > -void odp_hashnode_give(odph_table_t table, odph_hash_node *node) > +static void odp_hashnode_give(odph_table_t table, odph_hash_node *node) > odph_ > { > odph_hash_table_imp *tbl = (odph_hash_table_imp *)table; >
On 2 August 2016 at 04:44, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 08/02/16 10:54, Anders Roxell wrote: > >> Signed-off-by: Anders Roxell <anders.roxell@linaro.org> >> --- >> helper/hashtable.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/helper/hashtable.c b/helper/hashtable.c >> index 8bb1ae5..f17b80f 100644 >> --- a/helper/hashtable.c >> +++ b/helper/hashtable.c >> @@ -164,7 +164,7 @@ odph_table_t odph_hash_table_lookup(const char *name) >> * This hash algorithm is the most simple one, so we choose it as an >> DEMO >> * User can use any other algorithm, like CRC... >> */ >> -uint16_t odp_key_hash(void *key, uint32_t key_size) >> +static uint16_t odp_key_hash(void *key, uint32_t key_size) >> > odph_ > >> { >> register uint32_t hash = 0; >> uint32_t idx = (key_size == 0 ? 1 : key_size); >> @@ -181,7 +181,7 @@ uint16_t odp_key_hash(void *key, uint32_t key_size) >> /** >> * Get an available node from pool >> */ >> -odph_hash_node *odp_hashnode_take(odph_table_t table) >> +static odph_hash_node *odp_hashnode_take(odph_table_t table) >> { >> odph_hash_table_imp *tbl = (odph_hash_table_imp *)table; >> uint32_t idx; >> @@ -208,7 +208,7 @@ odph_hash_node *odp_hashnode_take(odph_table_t table) >> /** >> * Release an node to the pool >> */ >> -void odp_hashnode_give(odph_table_t table, odph_hash_node *node) >> +static void odp_hashnode_give(odph_table_t table, odph_hash_node *node) >> >> > odph_ Should this patch change the name as well ? The fix appears to be to correctly make internal functions static whist converting to a naming scheme is a separate fix > > { >> odph_hash_table_imp *tbl = (odph_hash_table_imp *)table; >> >> > > -- Mike Holmes Technical Manager - Linaro Networking Group Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs "Work should be fun and collaborative, the rest follows"
On 2 August 2016 at 17:27, Mike Holmes <mike.holmes@linaro.org> wrote: > On 2 August 2016 at 04:44, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > >> On 08/02/16 10:54, Anders Roxell wrote: >> >>> Signed-off-by: Anders Roxell <anders.roxell@linaro.org> >>> --- >>> helper/hashtable.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/helper/hashtable.c b/helper/hashtable.c >>> index 8bb1ae5..f17b80f 100644 >>> --- a/helper/hashtable.c >>> +++ b/helper/hashtable.c >>> @@ -164,7 +164,7 @@ odph_table_t odph_hash_table_lookup(const char *name) >>> * This hash algorithm is the most simple one, so we choose it as an >>> DEMO >>> * User can use any other algorithm, like CRC... >>> */ >>> -uint16_t odp_key_hash(void *key, uint32_t key_size) >>> +static uint16_t odp_key_hash(void *key, uint32_t key_size) >>> >> odph_ >> >>> { >>> register uint32_t hash = 0; >>> uint32_t idx = (key_size == 0 ? 1 : key_size); >>> @@ -181,7 +181,7 @@ uint16_t odp_key_hash(void *key, uint32_t key_size) >>> /** >>> * Get an available node from pool >>> */ >>> -odph_hash_node *odp_hashnode_take(odph_table_t table) >>> +static odph_hash_node *odp_hashnode_take(odph_table_t table) >>> { >>> odph_hash_table_imp *tbl = (odph_hash_table_imp *)table; >>> uint32_t idx; >>> @@ -208,7 +208,7 @@ odph_hash_node *odp_hashnode_take(odph_table_t table) >>> /** >>> * Release an node to the pool >>> */ >>> -void odp_hashnode_give(odph_table_t table, odph_hash_node *node) >>> +static void odp_hashnode_give(odph_table_t table, odph_hash_node *node) >>> >>> >> odph_ > > > Should this patch change the name as well ? I would say no. =) > The fix appears to be to correctly make internal functions static whist > converting to a naming scheme is a separate fix agree. Cheers, Anders > > >> >> { >>> odph_hash_table_imp *tbl = (odph_hash_table_imp *)table; >>> >>> >> >> > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs > "Work should be fun and collaborative, the rest follows"
On 08/03/16 22:21, Anders Roxell wrote: > On 2 August 2016 at 17:27, Mike Holmes <mike.holmes@linaro.org> wrote: >> On 2 August 2016 at 04:44, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >> >>> On 08/02/16 10:54, Anders Roxell wrote: >>> >>>> Signed-off-by: Anders Roxell <anders.roxell@linaro.org> >>>> --- >>>> helper/hashtable.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/helper/hashtable.c b/helper/hashtable.c >>>> index 8bb1ae5..f17b80f 100644 >>>> --- a/helper/hashtable.c >>>> +++ b/helper/hashtable.c >>>> @@ -164,7 +164,7 @@ odph_table_t odph_hash_table_lookup(const char *name) >>>> * This hash algorithm is the most simple one, so we choose it as an >>>> DEMO >>>> * User can use any other algorithm, like CRC... >>>> */ >>>> -uint16_t odp_key_hash(void *key, uint32_t key_size) >>>> +static uint16_t odp_key_hash(void *key, uint32_t key_size) >>>> >>> odph_ >>> >>>> { >>>> register uint32_t hash = 0; >>>> uint32_t idx = (key_size == 0 ? 1 : key_size); >>>> @@ -181,7 +181,7 @@ uint16_t odp_key_hash(void *key, uint32_t key_size) >>>> /** >>>> * Get an available node from pool >>>> */ >>>> -odph_hash_node *odp_hashnode_take(odph_table_t table) >>>> +static odph_hash_node *odp_hashnode_take(odph_table_t table) >>>> { >>>> odph_hash_table_imp *tbl = (odph_hash_table_imp *)table; >>>> uint32_t idx; >>>> @@ -208,7 +208,7 @@ odph_hash_node *odp_hashnode_take(odph_table_t table) >>>> /** >>>> * Release an node to the pool >>>> */ >>>> -void odp_hashnode_give(odph_table_t table, odph_hash_node *node) >>>> +static void odp_hashnode_give(odph_table_t table, odph_hash_node *node) >>>> >>>> >>> odph_ >> >> Should this patch change the name as well ? > I would say no. =) > >> The fix appears to be to correctly make internal functions static whist >> converting to a naming scheme is a separate fix > agree. > > Cheers, > Anders I don't mind if it will be 2 patches or 1 patch. If new issue was observed during review it has to be fixed with the same patch set. Or if you don't want/have time then you can file bug and we will take care about how will fix it. I would like to see one single patch here because you should not fix something partially and introduce patch lines with errors. If you touched that line than in my opinion it's logical to fix problem correct and completely. Problem from patch description is "to hide private functions". regards, Maxim, >> >>> { >>>> odph_hash_table_imp *tbl = (odph_hash_table_imp *)table; >>>> >>>> >>> >> >> -- >> Mike Holmes >> Technical Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs >> "Work should be fun and collaborative, the rest follows"
On 4 August 2016 at 02:30, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 08/03/16 22:21, Anders Roxell wrote: > >> On 2 August 2016 at 17:27, Mike Holmes <mike.holmes@linaro.org> wrote: >> >>> On 2 August 2016 at 04:44, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >>> >>> On 08/02/16 10:54, Anders Roxell wrote: >>>> >>>> Signed-off-by: Anders Roxell <anders.roxell@linaro.org> >>>>> --- >>>>> helper/hashtable.c | 6 +++--- >>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/helper/hashtable.c b/helper/hashtable.c >>>>> index 8bb1ae5..f17b80f 100644 >>>>> --- a/helper/hashtable.c >>>>> +++ b/helper/hashtable.c >>>>> @@ -164,7 +164,7 @@ odph_table_t odph_hash_table_lookup(const char >>>>> *name) >>>>> * This hash algorithm is the most simple one, so we choose it as an >>>>> DEMO >>>>> * User can use any other algorithm, like CRC... >>>>> */ >>>>> -uint16_t odp_key_hash(void *key, uint32_t key_size) >>>>> +static uint16_t odp_key_hash(void *key, uint32_t key_size) >>>>> >>>>> odph_ >>>> >>>> { >>>>> register uint32_t hash = 0; >>>>> uint32_t idx = (key_size == 0 ? 1 : key_size); >>>>> @@ -181,7 +181,7 @@ uint16_t odp_key_hash(void *key, uint32_t key_size) >>>>> /** >>>>> * Get an available node from pool >>>>> */ >>>>> -odph_hash_node *odp_hashnode_take(odph_table_t table) >>>>> +static odph_hash_node *odp_hashnode_take(odph_table_t table) >>>>> { >>>>> odph_hash_table_imp *tbl = (odph_hash_table_imp *)table; >>>>> uint32_t idx; >>>>> @@ -208,7 +208,7 @@ odph_hash_node *odp_hashnode_take(odph_table_t >>>>> table) >>>>> /** >>>>> * Release an node to the pool >>>>> */ >>>>> -void odp_hashnode_give(odph_table_t table, odph_hash_node *node) >>>>> +static void odp_hashnode_give(odph_table_t table, odph_hash_node >>>>> *node) >>>>> >>>>> >>>>> odph_ >>>> >>> >>> Should this patch change the name as well ? >>> >> I would say no. =) >> >> The fix appears to be to correctly make internal functions static whist >>> converting to a naming scheme is a separate fix >>> >> agree. >> >> Cheers, >> Anders >> > > I don't mind if it will be 2 patches or 1 patch. If new issue was observed > during review it has to be fixed with the same patch set. > Or if you don't want/have time then you can file bug and we will take care > about how will fix it. > > I would like to see one single patch here because you should not fix > something partially and introduce patch lines with errors. If you > touched that line than in my opinion it's logical to fix problem correct > and completely. Problem from patch description is "to hide private > functions". > Some times I agree that makes sense, but it is better to make a bug for a second issue if a patch clearly fixes the issues it claims to, without a balance we won't make progress if every patch requires fixing other issues that are not directly implicated. > > regards, > Maxim, > > > >>> { >>>> >>>>> odph_hash_table_imp *tbl = (odph_hash_table_imp *)table; >>>>> >>>>> >>>>> >>>> >>> -- >>> Mike Holmes >>> Technical Manager - Linaro Networking Group >>> Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM >>> SoCs >>> "Work should be fun and collaborative, the rest follows" >>> >> > -- Mike Holmes Technical Manager - Linaro Networking Group Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs "Work should be fun and collaborative, the rest follows"
diff --git a/helper/hashtable.c b/helper/hashtable.c index 8bb1ae5..f17b80f 100644 --- a/helper/hashtable.c +++ b/helper/hashtable.c @@ -164,7 +164,7 @@ odph_table_t odph_hash_table_lookup(const char *name) * This hash algorithm is the most simple one, so we choose it as an DEMO * User can use any other algorithm, like CRC... */ -uint16_t odp_key_hash(void *key, uint32_t key_size) +static uint16_t odp_key_hash(void *key, uint32_t key_size) { register uint32_t hash = 0; uint32_t idx = (key_size == 0 ? 1 : key_size); @@ -181,7 +181,7 @@ uint16_t odp_key_hash(void *key, uint32_t key_size) /** * Get an available node from pool */ -odph_hash_node *odp_hashnode_take(odph_table_t table) +static odph_hash_node *odp_hashnode_take(odph_table_t table) { odph_hash_table_imp *tbl = (odph_hash_table_imp *)table; uint32_t idx; @@ -208,7 +208,7 @@ odph_hash_node *odp_hashnode_take(odph_table_t table) /** * Release an node to the pool */ -void odp_hashnode_give(odph_table_t table, odph_hash_node *node) +static void odp_hashnode_give(odph_table_t table, odph_hash_node *node) { odph_hash_table_imp *tbl = (odph_hash_table_imp *)table;
Signed-off-by: Anders Roxell <anders.roxell@linaro.org> --- helper/hashtable.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) -- 2.1.4