Re: [PATCH v4] t: port helper/test-hashmap.c to unit-tests/t-hashmap.c

From: Christian Couder <christian.couder@gmail.com>
Date: Jul 31, 2024, 17:18 UTC
Message-ID: <CAP8UFD04uQbWhY0QDp+nC8VyBvpUs=tBDm=w4CzKK2GcwFZdGQ@mail.gmail.com>
In-Reply-To: <20240730115101.188100-1-author@example.com>

Hi Ghanshyam,

Not sure why the "From:" header of this email says:

"From: A U Thor <shyamthakkar001@gmail.com>"

Did you make changes to your config or something?

On Tue, Jul 30, 2024 at 1:52 PM A U Thor <shyamthakkar001@gmail.com> wrote:

>
> From: Ghanshyam Thakkar <shyamthakkar001@gmail.com>

Show 8 quoted lines

> Changes in v4:
> - update commit message to add a reference and fix typo
> - change 'int ignore_case' to 'unsigned int ignore_case'
> - make 'ignore_case' the last parameter in all the functions
> - update t_get() to add a testcase query for better ignore-case
>   checking
>
> Range-diff against v3:

Show 18 quoted lines

>     -+static void t_get(struct hashmap *map, int ignore_case)
>     ++static void t_get(struct hashmap *map, unsigned int ignore_case)
>      +{
>      +  struct test_entry *entry;
>      +  const char *key_val[][2] = { { "key1", "value1" },
>      +                               { "key2", "value2" },
>      +                               { "fooBarFrotz", "value3" },
>     -+                               { ignore_case ? "key4" : "foobarfrotz", "value4" } };
>     ++                               { ignore_case ? "TeNor" : "tenor",
>     ++                                 ignore_case ? "value4" : "value5" } };
>      +  const char *query[][2] = {
>      +          { ignore_case ? "Key1" : "key1", "value1" },
>      +          { ignore_case ? "keY2" : "key2", "value2" },
>     -+          { ignore_case ? "foobarfrotz" : "fooBarFrotz", "value3" }
>     ++          { ignore_case ? "FOObarFrotz" : "fooBarFrotz", "value3" },
>     ++          { ignore_case ? "TENOR" : "tenor",
>     ++            ignore_case ? "value4" : "value5" }
>      +  };

I suggested adding the following test case:

               { ignore_case ? "FOOBarFrotZ" : "foobarfrotz",
                 ignore_case ? : "value3" : "value4" }

to better check that things work well especially when not ignoring the case.

This is because, when not ignoring the case, there used to be a choice between { "fooBarFrotz", "value3" } and { "foobarfrotz", "value4" } that can be decided only by the case of the key in 'query'. But instead you removed that choice from 'key_val'.

And it seems to me that the new test case you added doesn't bring much value.

Maybe there was something wrong in what I suggested, but then please explain it.

The other changes look good to me and seem to address the suggestions made by others and me.

Thanks!