[PATCH] lib/string_map: fix return type of string_cmp

[PATCH] lib/string_map: fix return type of string_cmp

[David Bremner]

I can't figure out how checking the sign of a bool ever worked. The
following program demonstrates the problem (i.e. for me it prints 1).

 #include <stdio.h>
 #include <stdbool.h>
 int main(int argc, char **argv) {
    bool x;
    x = -1;
    printf("x = %d\n", x);
 }

This seems to be mandated by the C99 standard 6.3.1.2.
---

 I'd like to get this in a new point release ASAP.

 lib/string-map.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/string-map.c b/lib/string-map.c
index ad818207..a88404c7 100644
--- a/lib/string-map.c
+++ b/lib/string-map.c
@@ -106,7 +106,7 @@ _notmuch_string_map_sort (notmuch_string_map_t *map)
     map->sorted = true;
 }
 
-static bool
+static int
 string_cmp (const char *a, const char *b, bool exact)
 {
     if (exact)
-- 
2.20.1

Re: [PATCH] lib/string_map: fix return type of string_cmp

[Matt Armstrong]

David Bremner <david@tethera.net> writes:

> I can't figure out how checking the sign of a bool ever worked. The
> following program demonstrates the problem (i.e. for me it prints 1).
>
>  #include <stdio.h>
>  #include <stdbool.h>
>  int main(int argc, char **argv) {
>     bool x;
>     x = -1;
>     printf("x = %d\n", x);
>  }
>
> This seems to be mandated by the C99 standard 6.3.1.2.

Perhaps it never did work?  It looks like this function to date is used
only for properties, where I assume each message has just one or two?
With just a few elements a buggy binary search can do surprisingly well.
Don't ask me how I know this.  ;-)

Perhaps some unit tests for _notmuch_string_map would be worthwhile?


> ---
>
>  I'd like to get this in a new point release ASAP.
>
>  lib/string-map.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/string-map.c b/lib/string-map.c
> index ad818207..a88404c7 100644
> --- a/lib/string-map.c
> +++ b/lib/string-map.c
> @@ -106,7 +106,7 @@ _notmuch_string_map_sort (notmuch_string_map_t *map)
>      map->sorted = true;
>  }
>  
> -static bool
> +static int
>  string_cmp (const char *a, const char *b, bool exact)
>  {
>      if (exact)
> -- 

Looks good.

Re: [PATCH] lib/string_map: fix return type of string_cmp

[David Bremner]

Matt Armstrong <marmstrong@google.com> writes:

> Perhaps some unit tests for _notmuch_string_map would be worthwhile?
>

Thanks for the feedback. At first I thought this would be too hard/intrusive,
since the string_map functions are not exported, but I realized
n_message_property_get is a thin wrapper on _notmuch_string_map_get, so
I added the following test. Most of the searches fail before the patch.

test_begin_subtest "testing string map binary search (via message properties)"
cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
{
   char *keys[] = {"a", "b", "c", "d", "e", NULL};
   for (int i=0; keys[i]; i++)
       EXPECT0(notmuch_message_add_property (message, keys[i], keys[i]));

   for (int i=0; keys[i]; i++) {
      EXPECT0(notmuch_message_get_property (message, keys[i], &val));
      printf("%s = %s\n", keys[i], val);
   }

   for (int i=0; keys[i]; i++)
      EXPECT0(notmuch_message_remove_property (message, keys[i], keys[i]));
}
EOF
cat <<EOF > EXPECTED
== stdout ==
a = a
b = b
c = c
d = d
e = e
== stderr ==
EOF
test_expect_equal_file EXPECTED OUTPUT

Re: [PATCH] lib/string_map: fix return type of string_cmp

[Matt Armstrong]

David Bremner <david@tethera.net> writes:

> Matt Armstrong <marmstrong@google.com> writes:
>
>> Perhaps some unit tests for _notmuch_string_map would be worthwhile?
>>
>
> Thanks for the feedback. At first I thought this would be too hard/intrusive,
> since the string_map functions are not exported, but I realized
> n_message_property_get is a thin wrapper on _notmuch_string_map_get, so
> I added the following test. Most of the searches fail before the patch.
>
> test_begin_subtest "testing string map binary search (via message properties)"
> cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
> {
>    char *keys[] = {"a", "b", "c", "d", "e", NULL};
>    for (int i=0; keys[i]; i++)
>        EXPECT0(notmuch_message_add_property (message, keys[i], keys[i]));
>
>    for (int i=0; keys[i]; i++) {
>       EXPECT0(notmuch_message_get_property (message, keys[i], &val));
>       printf("%s = %s\n", keys[i], val);
>    }
>
>    for (int i=0; keys[i]; i++)
>       EXPECT0(notmuch_message_remove_property (message, keys[i], keys[i]));
> }

Nice.  The test could even go further and verify that get_property fails
after the properties are removed.