helper/hashtable: use static to hide private functions

Message ID 1470124474-3489-1-git-send-email-anders.roxell@linaro.org
State New
Headers show

Commit Message

Anders Roxell Aug. 2, 2016, 7:54 a.m.
Signed-off-by: Anders Roxell <anders.roxell@linaro.org>

---
 helper/hashtable.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.1.4

Comments

Maxim Uvarov Aug. 2, 2016, 8:44 a.m. | #1
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;

>
Mike Holmes Aug. 2, 2016, 3:27 p.m. | #2
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"
Anders Roxell Aug. 3, 2016, 7:21 p.m. | #3
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"
Maxim Uvarov Aug. 4, 2016, 6:31 a.m. | #4
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"
Mike Holmes Aug. 5, 2016, 4:35 p.m. | #5
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"

Patch

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;