notmuch on Solaris

notmuch on Solaris

[Vladimir.Marek]

Hi,

In order to make notmuch compilable on Solaris, I had to make some
changes in the source code. Some changes are not yet ready to be
included to official tree, some I believe are. So I took first few to
see if I'm doing anything which could be accepted and hopefully decrease
number of local patches I have to hold. Comments are mostly welcome.

Thank you
-- 
	Vlad

[PATCH 1/4] Make configure use /bin/bash instead of /bin/sh

[Vladimir.Marek]

From: Vladimir Marek <vlmarek@volny.cz>

Posix /bin/sh is not capable of running this configure and fails.

Signed-off-by: Vladimir Marek <vlmarek@volny.cz>
---
 configure |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/configure b/configure
index 71981b7..6870341 100755
--- a/configure
+++ b/configure
@@ -1,4 +1,4 @@
-#! /bin/sh
+#! /bin/bash
 
 # Store original IFS value so it can be changed (and restored) in many places.
 readonly DEFAULT_IFS=$IFS
-- 
1.7.3.2

[PATCH 2/4] dirent->d_type not available on Soalris

[Vladimir.Marek]

From: Vladimir Marek <vlmarek@volny.cz>

The inspiration was taken from similar issue in mutt:
http://does-not-exist.org/mail-archives/mutt-dev/msg11290.html

Signed-off-by: Vladimir Marek <vlmarek@volny.cz>
---
 notmuch-new.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 4f13535..20bc580 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -21,6 +21,7 @@
 #include "notmuch-client.h"
 
 #include <unistd.h>
+#include <sys/types.h>
 
 typedef struct _filename_node {
     char *filename;
@@ -165,9 +166,12 @@ static int
 _entries_resemble_maildir (struct dirent **entries, int count)
 {
     int i, found = 0;
+    struct stat statbuf;
 
     for (i = 0; i < count; i++) {
-	if (entries[i]->d_type != DT_DIR && entries[i]->d_type != DT_UNKNOWN)
+	if (stat(entries[i]->d_name, &statbuf) == -1)
+		continue;
+	if (! S_ISDIR(statbuf.st_mode))
 	    continue;
 
 	if (strcmp(entries[i]->d_name, "new") == 0 ||
@@ -258,6 +262,7 @@ add_files_recursive (notmuch_database_t *notmuch,
     struct stat st;
     notmuch_bool_t is_maildir, new_directory;
     const char **tag;
+    struct stat statbuf;
 
     if (stat (path, &st)) {
 	fprintf (stderr, "Error reading directory %s: %s\n",
@@ -321,6 +326,9 @@ add_files_recursive (notmuch_database_t *notmuch,
 
 	entry = fs_entries[i];
 
+	if (stat(entry->d_name, &statbuf) == -1)
+		continue;
+
 	/* We only want to descend into directories.
 	 * But symlinks can be to directories too, of course.
 	 *
@@ -328,9 +336,8 @@ add_files_recursive (notmuch_database_t *notmuch,
 	 * scandir results, then it might be a directory (and if not,
 	 * then we'll stat and return immediately in the next level of
 	 * recursion). */
-	if (entry->d_type != DT_DIR &&
-	    entry->d_type != DT_LNK &&
-	    entry->d_type != DT_UNKNOWN)
+	if (!(statbuf.st_mode & S_IFDIR) &&
+	    !(statbuf.st_mode & S_IFLNK))
 	{
 	    continue;
 	}
@@ -427,7 +434,7 @@ add_files_recursive (notmuch_database_t *notmuch,
 	 *
 	 * In either case, a stat does the trick.
 	 */
-	if (entry->d_type == DT_LNK || entry->d_type == DT_UNKNOWN) {
+	if (stat(entry->d_name, &statbuf) == -1 || statbuf.st_mode & S_IFLNK) {
 	    int err;
 
 	    next = talloc_asprintf (notmuch, "%s/%s", path, entry->d_name);
@@ -443,7 +450,7 @@ add_files_recursive (notmuch_database_t *notmuch,
 
 	    if (! S_ISREG (st.st_mode))
 		continue;
-	} else if (entry->d_type != DT_REG) {
+	} else if ( statbuf.st_mode & S_IFREG) {
 	    continue;
 	}
 
-- 
1.7.3.2

[PATCH 3/4] Private strsep implementation

[Vladimir.Marek]

From: Vladimir Marek <vlmarek@volny.cz>

strsep is not available on Solaris 10, so we stole the one used by mutt.

Signed-off-by: Vladimir Marek <vlmarek@volny.cz>
---
 compat/Makefile.local |    4 +++
 compat/compat.h       |    4 +++
 compat/have_strsep.c  |   10 +++++++
 compat/strsep.c       |   65 +++++++++++++++++++++++++++++++++++++++++++++++++
 configure             |   21 ++++++++++++++-
 5 files changed, 102 insertions(+), 2 deletions(-)
 create mode 100644 compat/have_strsep.c
 create mode 100644 compat/strsep.c

diff --git a/compat/Makefile.local b/compat/Makefile.local
index 13f16cd..2c4f65f 100644
--- a/compat/Makefile.local
+++ b/compat/Makefile.local
@@ -13,4 +13,8 @@ ifneq ($(HAVE_STRCASESTR),1)
 notmuch_compat_srcs += $(dir)/strcasestr.c
 endif
 
+ifneq ($(HAVE_STRSEP),1)
+notmuch_compat_srcs += $(dir)/strsep.c
+endif
+
 SRCS := $(SRCS) $(notmuch_compat_srcs)
diff --git a/compat/compat.h b/compat/compat.h
index b2e2736..cf8d56d 100644
--- a/compat/compat.h
+++ b/compat/compat.h
@@ -46,6 +46,10 @@ getdelim (char **lineptr, size_t *n, int delimiter, FILE *fp);
 char* strcasestr(const char *haystack, const char *needle);
 #endif /* !HAVE_STRCASESTR */
 
+#if !HAVE_STRSEP
+char *strsep(char **stringp, const char *delim);
+#endif /* !HAVE_STRSEP */
+
 /* Silence gcc warnings about unused results.  These warnings exist
  * for a reason; any use of this needs to be justified. */
 #ifdef __GNUC__
diff --git a/compat/have_strsep.c b/compat/have_strsep.c
new file mode 100644
index 0000000..5bd396c
--- /dev/null
+++ b/compat/have_strsep.c
@@ -0,0 +1,10 @@
+#define _GNU_SOURCE
+#include <string.h>
+
+int main()
+{
+    char *found;
+    char **stringp, const char *delim;
+
+    found = strsep(stringp, delim);
+}
diff --git a/compat/strsep.c b/compat/strsep.c
new file mode 100644
index 0000000..78ab9e7
--- /dev/null
+++ b/compat/strsep.c
@@ -0,0 +1,65 @@
+/* Copyright (C) 1992, 93, 96, 97, 98, 99, 2004 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, write to the Free
+   Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
+   02111-1307 USA.  */
+
+#include <string.h>
+
+/* Taken from glibc 2.6.1 */
+
+char *strsep (char **stringp, const char *delim)
+{
+  char *begin, *end;
+
+  begin = *stringp;
+  if (begin == NULL)
+    return NULL;
+
+  /* A frequent case is when the delimiter string contains only one
+     character.  Here we don't need to call the expensive `strpbrk'
+     function and instead work using `strchr'.  */
+  if (delim[0] == '\0' || delim[1] == '\0')
+    {
+      char ch = delim[0];
+
+      if (ch == '\0')
+	end = NULL;
+      else
+	{
+	  if (*begin == ch)
+	    end = begin;
+	  else if (*begin == '\0')
+	    end = NULL;
+	  else
+	    end = strchr (begin + 1, ch);
+	}
+    }
+  else
+    /* Find the end of the token.  */
+    end = strpbrk (begin, delim);
+
+  if (end)
+    {
+      /* Terminate the token and set *STRINGP past NUL character.  */
+      *end++ = '\0';
+      *stringp = end;
+    }
+  else
+    /* No more delimiters; this is the last token.  */
+    *stringp = NULL;
+
+  return begin;
+}
diff --git a/configure b/configure
index 6870341..a06df7c 100755
--- a/configure
+++ b/configure
@@ -486,6 +486,17 @@ else
 fi
 rm -f compat/have_strcasestr
 
+printf "Checking for strsep... "
+if ${CC} -o compat/have_strsep "$srcdir"/compat/have_strsep > /dev/null 2>&1
+then
+    printf "Yes.\n"
+    have_strsep=1
+else
+    printf "No (will use our own instead).\n"
+    have_strsep=0
+fi
+rm -f compat/have_strsep
+
 printf "int main(void){return 0;}\n" > minimal.c
 
 printf "Checking for rpath support... "
@@ -645,6 +656,10 @@ HAVE_GETLINE = ${have_getline}
 # build its own version)
 HAVE_STRCASESTR = ${have_strcasestr}
 
+# Whether the strsep function is available (if not, then notmuch will
+# build its own version)
+HAVE_STRSEP = ${have_strsep}
+
 # Supported platforms (so far) are: LINUX, MACOSX, SOLARIS
 PLATFORM = ${platform}
 
@@ -689,10 +704,12 @@ WITH_ZSH = ${WITH_ZSH}
 # Combined flags for compiling and linking against all of the above
 CONFIGURE_CFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)      \\
 		   \$(TALLOC_CFLAGS) -DHAVE_VALGRIND=\$(HAVE_VALGRIND)   \\
-		   \$(VALGRIND_CFLAGS) -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)
+		   \$(VALGRIND_CFLAGS) -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR) \\
+		   -DHAVE_STRSEP=\$(HAVE_STRSEP)
 CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\
 		     \$(TALLOC_CFLAGS) -DHAVE_VALGRIND=\$(HAVE_VALGRIND) \\
 		     \$(VALGRIND_CFLAGS) \$(XAPIAN_CXXFLAGS)             \\
-                     -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)
+		     -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR) \\
+		     -DHAVE_STRSEP=\$(HAVE_STRSEP)
 CONFIGURE_LDFLAGS =  \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(XAPIAN_LDFLAGS)
 EOF
-- 
1.7.3.2

[PATCH 4/4] Explicitly type void* pointers

[Vladimir.Marek]

From: Vladimir Marek <vlmarek@volny.cz>


Signed-off-by: Vladimir Marek <vlmarek@volny.cz>
---
 lib/database.cc |    2 +-
 lib/message.cc  |    2 +-
 lib/thread.cc   |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 16c4354..3c82632 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1361,7 +1361,7 @@ _resolve_message_id_to_thread_id (notmuch_database_t *notmuch,
 	return status;
 
     if (message) {
-	*thread_id_ret = talloc_steal (ctx,
+	*thread_id_ret = (const char*)talloc_steal (ctx,
 				       notmuch_message_get_thread_id (message));
 
 	notmuch_message_destroy (message);
diff --git a/lib/message.cc b/lib/message.cc
index 0075425..d56d442 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -220,7 +220,7 @@ _notmuch_message_create_for_message_id (notmuch_database_t *notmuch,
 									    message_id,
 									    &message);
     if (message)
-	return talloc_steal (notmuch, message);
+	return (notmuch_message_t*) talloc_steal (notmuch, message);
     else if (*status_ret)
 	return NULL;
 
diff --git a/lib/thread.cc b/lib/thread.cc
index e976d64..d41ff3e 100644
--- a/lib/thread.cc
+++ b/lib/thread.cc
@@ -225,7 +225,7 @@ _thread_add_message (notmuch_thread_t *thread,
     char *clean_author;
 
     _notmuch_message_list_add_message (thread->message_list,
-				       talloc_steal (thread, message));
+				       (_notmuch_message*)talloc_steal (thread, message));
     thread->total_messages++;
 
     g_hash_table_insert (thread->message_hash,
-- 
1.7.3.2

Re: [PATCH 4/4] Explicitly type void* pointers

[Jani Nikula]

[-- Attachment #1: Type: text/plain, Size: 2153 bytes --]

Hi, does notmuch not compile without this? IIRC talloc_steal is a macro
that's supposed to provide type safety (at least with GCC), and I'd be
hesitant about adding the casts. Please look in your talloc.h.

BR,
Jani.

On Apr 9, 2012 1:19 PM, <Vladimir.Marek@oracle.com> wrote:
>
> From: Vladimir Marek <vlmarek@volny.cz>
>
>
> Signed-off-by: Vladimir Marek <vlmarek@volny.cz>
> ---
>  lib/database.cc |    2 +-
>  lib/message.cc  |    2 +-
>  lib/thread.cc   |    2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/lib/database.cc b/lib/database.cc
> index 16c4354..3c82632 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -1361,7 +1361,7 @@ _resolve_message_id_to_thread_id
(notmuch_database_t *notmuch,
>        return status;
>
>     if (message) {
> -       *thread_id_ret = talloc_steal (ctx,
> +       *thread_id_ret = (const char*)talloc_steal (ctx,
>                                       notmuch_message_get_thread_id
(message));
>
>        notmuch_message_destroy (message);
> diff --git a/lib/message.cc b/lib/message.cc
> index 0075425..d56d442 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -220,7 +220,7 @@ _notmuch_message_create_for_message_id
(notmuch_database_t *notmuch,
>
 message_id,
>
 &message);
>     if (message)
> -       return talloc_steal (notmuch, message);
> +       return (notmuch_message_t*) talloc_steal (notmuch, message);
>     else if (*status_ret)
>        return NULL;
>
> diff --git a/lib/thread.cc b/lib/thread.cc
> index e976d64..d41ff3e 100644
> --- a/lib/thread.cc
> +++ b/lib/thread.cc
> @@ -225,7 +225,7 @@ _thread_add_message (notmuch_thread_t *thread,
>     char *clean_author;
>
>     _notmuch_message_list_add_message (thread->message_list,
> -                                      talloc_steal (thread, message));
> +                                      (_notmuch_message*)talloc_steal
(thread, message));
>     thread->total_messages++;
>
>     g_hash_table_insert (thread->message_hash,
> --
> 1.7.3.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[-- Attachment #2: Type: text/html, Size: 3252 bytes --]

Re: [PATCH 1/4] Make configure use /bin/bash instead of /bin/sh

[Jani Nikula]

[-- Attachment #1: Type: text/plain, Size: 929 bytes --]

On Apr 9, 2012 1:18 PM, <Vladimir.Marek@oracle.com> wrote:
>
> From: Vladimir Marek <vlmarek@volny.cz>
>
> Posix /bin/sh is not capable of running this configure and fails.

What fails? What would it take to make this work on posix sh instead?

The tests do require bash, but generally I think it would be preferable to
not depend on bash to build.

BR,
Jani.

>
> Signed-off-by: Vladimir Marek <vlmarek@volny.cz>
> ---
>  configure |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/configure b/configure
> index 71981b7..6870341 100755
> --- a/configure
> +++ b/configure
> @@ -1,4 +1,4 @@
> -#! /bin/sh
> +#! /bin/bash
>
>  # Store original IFS value so it can be changed (and restored) in many
places.
>  readonly DEFAULT_IFS=$IFS
> --
> 1.7.3.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[-- Attachment #2: Type: text/html, Size: 1455 bytes --]

Re: [PATCH 2/4] dirent->d_type not available on Soalris

[Jani Nikula]

[-- Attachment #1: Type: text/plain, Size: 3305 bytes --]

On Apr 9, 2012 1:18 PM, <Vladimir.Marek@oracle.com> wrote:
>
> From: Vladimir Marek <vlmarek@volny.cz>
>
> The inspiration was taken from similar issue in mutt:
> http://does-not-exist.org/mail-archives/mutt-dev/msg11290.html
>
> Signed-off-by: Vladimir Marek <vlmarek@volny.cz>
> ---
>  notmuch-new.c |   19 +++++++++++++------
>  1 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/notmuch-new.c b/notmuch-new.c
> index 4f13535..20bc580 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -21,6 +21,7 @@
>  #include "notmuch-client.h"
>
>  #include <unistd.h>
> +#include <sys/types.h>
>
>  typedef struct _filename_node {
>     char *filename;
> @@ -165,9 +166,12 @@ static int
>  _entries_resemble_maildir (struct dirent **entries, int count)
>  {
>     int i, found = 0;
> +    struct stat statbuf;
>
>     for (i = 0; i < count; i++) {
> -       if (entries[i]->d_type != DT_DIR && entries[i]->d_type !=
DT_UNKNOWN)
> +       if (stat(entries[i]->d_name, &statbuf) == -1)
> +               continue;
> +       if (! S_ISDIR(statbuf.st_mode))

Notmuch new is possibly one of the most performance critical bits for
people with, uh, much mail. The performance impact of the new syscalls
should be measured. (Can't do this myself atm.)

BR,
Jani.

>            continue;
>
>        if (strcmp(entries[i]->d_name, "new") == 0 ||
> @@ -258,6 +262,7 @@ add_files_recursive (notmuch_database_t *notmuch,
>     struct stat st;
>     notmuch_bool_t is_maildir, new_directory;
>     const char **tag;
> +    struct stat statbuf;
>
>     if (stat (path, &st)) {
>        fprintf (stderr, "Error reading directory %s: %s\n",
> @@ -321,6 +326,9 @@ add_files_recursive (notmuch_database_t *notmuch,
>
>        entry = fs_entries[i];
>
> +       if (stat(entry->d_name, &statbuf) == -1)
> +               continue;
> +
>        /* We only want to descend into directories.
>         * But symlinks can be to directories too, of course.
>         *
> @@ -328,9 +336,8 @@ add_files_recursive (notmuch_database_t *notmuch,
>         * scandir results, then it might be a directory (and if not,
>         * then we'll stat and return immediately in the next level of
>         * recursion). */
> -       if (entry->d_type != DT_DIR &&
> -           entry->d_type != DT_LNK &&
> -           entry->d_type != DT_UNKNOWN)
> +       if (!(statbuf.st_mode & S_IFDIR) &&
> +           !(statbuf.st_mode & S_IFLNK))
>        {
>            continue;
>        }
> @@ -427,7 +434,7 @@ add_files_recursive (notmuch_database_t *notmuch,
>         *
>         * In either case, a stat does the trick.
>         */
> -       if (entry->d_type == DT_LNK || entry->d_type == DT_UNKNOWN) {
> +       if (stat(entry->d_name, &statbuf) == -1 || statbuf.st_mode &
S_IFLNK) {
>            int err;
>
>            next = talloc_asprintf (notmuch, "%s/%s", path, entry->d_name);
> @@ -443,7 +450,7 @@ add_files_recursive (notmuch_database_t *notmuch,
>
>            if (! S_ISREG (st.st_mode))
>                continue;
> -       } else if (entry->d_type != DT_REG) {
> +       } else if ( statbuf.st_mode & S_IFREG) {
>            continue;
>        }
>
> --
> 1.7.3.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[-- Attachment #2: Type: text/html, Size: 4767 bytes --]

Re: [PATCH 1/4] Make configure use /bin/bash instead of /bin/sh

[Vladimir Marek]

> > Posix /bin/sh is not capable of running this configure and fails.
> 
> What fails? What would it take to make this work on posix sh instead?
> 
> The tests do require bash, but generally I think it would be preferable to
> not depend on bash to build.

Well I gave it a quick stab. This is not posix:

BLAH=$( ... )
BLAH=$(( ... ))
${option%=*}
${option%%=*}
${option#=*}
${option##=*}

First two cases are easy to replace by `...` resp `expr ...`. The rest
leads to external utility like sed. The dirtiest part of configure is
parsing the commandline arguments, but that could be replaced by
/usr/bin/getopts.

If it is appealing way of doing that, I can rework my patch and submit
it for consideration.

Thank you
-- 
	Vlad

Re: [PATCH 2/4] dirent->d_type not available on Soalris

[Vladimir Marek]

> Notmuch new is possibly one of the most performance critical bits for
> people with, uh, much mail. The performance impact of the new syscalls
> should be measured. (Can't do this myself atm.)

Fair enough. Is there some performance test suite? Another way would be
to make this compile time option set by configure. Used only when the
system in question does not have dirent->d_type member.

-- 
	Vlad

Re: [PATCH 2/4] dirent->d_type not available on Soalris

[Adam Wolfe Gordon]

On Mon, Apr 9, 2012 at 09:12, Vladimir Marek <Vladimir.Marek@oracle.com> wrote:
> Fair enough. Is there some performance test suite? Another way would be
> to make this compile time option set by configure. Used only when the
> system in question does not have dirent->d_type member.

I like the idea of making it configurable. From the Linux readdir(3) man page:

> Under glibc, programs can check  for  the  availability
> of  the  fields  not defined in POSIX.1 by testing whether
> the macros _DIRENT_HAVE_D_NAMLEN,
> _DIRENT_HAVE_D_RECLEN, _DIRENT_HAVE_D_OFF,
> or  _DIRENT_HAVE_D_TYPE  are defined.

I read this as meaning that we can just test for _DIRENT_HAVE_D_TYPE
instead of adding our own define.

I expect the stat system calls will provide some overhead, and there's
no real need to pay that overhead on systems where it isn't necessary.

-- Adam

[PATCH 2/4] dirent->d_type not available on Soalris

[Vladimir.Marek]

From: Vladimir Marek <vlmarek@volny.cz>

The inspiration was taken from similar issue in mutt:
http://does-not-exist.org/mail-archives/mutt-dev/msg11290.html

Signed-off-by: Vladimir Marek <vlmarek@volny.cz>
---
 notmuch-new.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 4f13535..3d265bd 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -21,6 +21,9 @@
 #include "notmuch-client.h"
 
 #include <unistd.h>
+#ifndef _DIRENT_HAVE_D_TYPE
+#include <sys/types.h>
+#endif
 
 typedef struct _filename_node {
     char *filename;
@@ -167,7 +170,14 @@ _entries_resemble_maildir (struct dirent **entries, int count)
     int i, found = 0;
 
     for (i = 0; i < count; i++) {
+#ifdef _DIRENT_HAVE_D_TYPE
 	if (entries[i]->d_type != DT_DIR && entries[i]->d_type != DT_UNKNOWN)
+#else
+	struct stat statbuf;
+	if (stat(entries[i]->d_name, &statbuf) == -1)
+		continue;
+	if (! S_ISDIR(statbuf.st_mode))
+#endif
 	    continue;
 
 	if (strcmp(entries[i]->d_name, "new") == 0 ||
@@ -258,6 +268,9 @@ add_files_recursive (notmuch_database_t *notmuch,
     struct stat st;
     notmuch_bool_t is_maildir, new_directory;
     const char **tag;
+#ifndef _DIRENT_HAVE_D_TYPE
+    struct stat statbuf;
+#endif
 
     if (stat (path, &st)) {
 	fprintf (stderr, "Error reading directory %s: %s\n",
@@ -328,9 +341,16 @@ add_files_recursive (notmuch_database_t *notmuch,
 	 * scandir results, then it might be a directory (and if not,
 	 * then we'll stat and return immediately in the next level of
 	 * recursion). */
+#ifdef _DIRENT_HAVE_D_TYPE
 	if (entry->d_type != DT_DIR &&
 	    entry->d_type != DT_LNK &&
 	    entry->d_type != DT_UNKNOWN)
+#else
+	if (stat(entry->d_name, &statbuf) == -1)
+		continue;
+	if (!(statbuf.st_mode & S_IFDIR) &&
+	    !(statbuf.st_mode & S_IFLNK))
+#endif
 	{
 	    continue;
 	}
@@ -427,7 +447,11 @@ add_files_recursive (notmuch_database_t *notmuch,
 	 *
 	 * In either case, a stat does the trick.
 	 */
+#ifdef _DIRENT_HAVE_D_TYPE
 	if (entry->d_type == DT_LNK || entry->d_type == DT_UNKNOWN) {
+#else
+	if (stat(entry->d_name, &statbuf) == -1 || statbuf.st_mode & S_IFLNK) {
+#endif
 	    int err;
 
 	    next = talloc_asprintf (notmuch, "%s/%s", path, entry->d_name);
@@ -443,7 +467,11 @@ add_files_recursive (notmuch_database_t *notmuch,
 
 	    if (! S_ISREG (st.st_mode))
 		continue;
+#ifdef _DIRENT_HAVE_D_TYPE
 	} else if (entry->d_type != DT_REG) {
+#else
+	} else if (statbuf.st_mode & S_IFREG) {
+#endif
 	    continue;
 	}
 
-- 
1.7.3.2

Re: [PATCH 4/4] Explicitly type void* pointers

[Vladimir Marek]

Hi,

> Hi, does notmuch not compile without this? IIRC talloc_steal is a macro
> that's supposed to provide type safety (at least with GCC), and I'd be
> hesitant about adding the casts. Please look in your talloc.h.

It does not compile. It might be that I'm using Sun/Oracle CC instead of
gcc. The error looks like this:

"lib/database.cc", line 1368: Error: Cannot assign void* to const char*.


When looking into talloc documentation, the definition seems to be:

void* talloc_steal ( const void * new_ctx, const void * ptr )

http://talloc.samba.org/talloc/doc/html/group__talloc.html#gaccc66139273e727183fb5bdda11ef82c


When looking into talloc.h, it says:

/* try to make talloc_set_destructor() and talloc_steal() type safe,
   if we have a recent gcc */

So, maybe the way to satisfy everyone would be:

notmuch_message_t *tmp_message = message;
talloc_steal(notmuch, tmp_message);
return(tmp_message);

Or alternatively,

#if (__GNUC__ >= 3)
       return talloc_steal (notmuch, message);
#else
       return (notmuch_message_t*) talloc_steal (notmuch, message);
#fi


Of course I'm happy either way :)

Thank you
-- 
	Vlad

Re: [PATCH 4/4] Explicitly type void* pointers

[Jani Nikula]

Vladimir Marek <Vladimir.Marek@Oracle.COM> writes:

> Hi,
>
>> Hi, does notmuch not compile without this? IIRC talloc_steal is a macro
>> that's supposed to provide type safety (at least with GCC), and I'd be
>> hesitant about adding the casts. Please look in your talloc.h.
>
> It does not compile. It might be that I'm using Sun/Oracle CC instead of
> gcc. The error looks like this:
>
> "lib/database.cc", line 1368: Error: Cannot assign void* to const char*.

In general, that's not a difference in the C++ compilers. You can't
assign 'void *' to 'T *' in C++.

> When looking into talloc documentation, the definition seems to be:
>
> void* talloc_steal ( const void * new_ctx, const void * ptr )
>
> http://talloc.samba.org/talloc/doc/html/group__talloc.html#gaccc66139273e727183fb5bdda11ef82c
>
>
> When looking into talloc.h, it says:
>
> /* try to make talloc_set_destructor() and talloc_steal() type safe,
>    if we have a recent gcc */

It just so happens that the trick for type safety fixes the problem for
recent GCC by having an explicit cast.

> So, maybe the way to satisfy everyone would be:
>
> notmuch_message_t *tmp_message = message;
> talloc_steal(notmuch, tmp_message);
> return(tmp_message);
>
> Or alternatively,
>
> #if (__GNUC__ >= 3)
>        return talloc_steal (notmuch, message);
> #else
>        return (notmuch_message_t*) talloc_steal (notmuch, message);
> #fi
>
>
> Of course I'm happy either way :)

I'm throwing in a third alternative below. Does it work for you? I think
it's both prettier and uglier than the above at the same time! ;)

A middle ground would be to change the callers to use
"notmuch_talloc_steal", and just #define notmuch_talloc_steal
talloc_steal if __GNUC__ >= 3.

One could argue upstream talloc should have this, but OTOH it's a C
library.

BR,
Jani.


diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index ea836f7..83b46e8 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -499,4 +499,22 @@ _notmuch_filenames_create (const void *ctx,
 
 NOTMUCH_END_DECLS
 
+#ifdef __cplusplus
+/* Implicit typecast from 'void *' to 'T *' is okay in C, but not in
+ * C++. In talloc_steal, an explicit cast is provided for type safety
+ * in some GCC versions. Otherwise, a cast is required. Provide a
+ * template function for this to maintain type safety, and redefine
+ * talloc_steal to use it.
+ */
+#if !(__GNUC__ >= 3)
+template <class T>
+T *notmuch_talloc_steal(const void *new_ctx, const T *ptr)
+{
+    return static_cast<T*>(talloc_steal(new_ctx, ptr));
+}
+#undef talloc_steal
+#define talloc_steal notmuch_talloc_steal
+#endif
+#endif
+
 #endif

Re: [PATCH 4/4] Explicitly type void* pointers

[Vladimir Marek]

[...]

> I'm throwing in a third alternative below. Does it work for you? I think
> it's both prettier and uglier than the above at the same time! ;)
> 
> A middle ground would be to change the callers to use
> "notmuch_talloc_steal", and just #define notmuch_talloc_steal
> talloc_steal if __GNUC__ >= 3.
> 
> One could argue upstream talloc should have this, but OTOH it's a C
> library.

That's a nice trick, and it indeed works for me. And I like it more than
what I suggested.

Thank you
-- 
	Vlad

Re: [PATCH 1/4] Make configure use /bin/bash instead of /bin/sh

[Tomi Ollila]

On Mon, Apr 09 2012, Vladimir Marek wrote:

>> > Posix /bin/sh is not capable of running this configure and fails.
>> 
>> What fails? What would it take to make this work on posix sh instead?
>> 
>> The tests do require bash, but generally I think it would be preferable to
>> not depend on bash to build.
>
> Well I gave it a quick stab. This is not posix:
>
> BLAH=$( ... )
> BLAH=$(( ... ))
> ${option%=*}
> ${option%%=*}
> ${option#=*}
> ${option##=*}

According to 

http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html

all of those are part of Shell Command Language...

Does the configure script work if you replace /bin/sh with /bin/ksh
in your Solaris box

If yes, something like the following could be added to the beginning
of 'configure'

option=option=value
if test ! x"${option$*=}" = x"value" 2>/dev/null; then
	if test x"${PREVENT_LOOPING-}" = x; then
        	PREVENT_LOOPING=true; export PREVENT_LOOPING
                test ! -x /bin/ksh || exec /bin/ksh "$0" "$@"
                test ! -x /bin/bash || exec /bin/bash "$0" "$@"
        fi
        echo "Cannot find compatible shell for '$0'" >&2
        exit 1
fi



>
> First two cases are easy to replace by `...` resp `expr ...`. The rest
> leads to external utility like sed. The dirtiest part of configure is
> parsing the commandline arguments, but that could be replaced by
> /usr/bin/getopts.
>
> If it is appealing way of doing that, I can rework my patch and submit
> it for consideration.
>
> Thank you
> -- 
> 	Vlad
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/4] Make configure use /bin/bash instead of /bin/sh

[Vladimir Marek]

Hi,

> >> > Posix /bin/sh is not capable of running this configure and fails.
> >> 
> >> What fails? What would it take to make this work on posix sh instead?
> >> 
> >> The tests do require bash, but generally I think it would be preferable to
> >> not depend on bash to build.
> >
> > Well I gave it a quick stab. This is not posix:
> >
> > BLAH=$( ... )
> > BLAH=$(( ... ))
> > ${option%=*}
> > ${option%%=*}
> > ${option#=*}
> > ${option##=*}
> 
> According to 
> 
> http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html
> 
> all of those are part of Shell Command Language...

Hmm, you are right. The solaris /bin/sh is older revision of posix.



> Does the configure script work if you replace /bin/sh with /bin/ksh
> in your Solaris box

yes, it does work if executed by /bin/bash or /bin/ksh


> If yes, something like the following could be added to the beginning
> of 'configure'
> 
> option=option=value
> if test ! x"${option$*=}" = x"value" 2>/dev/null; then
> 	if test x"${PREVENT_LOOPING-}" = x; then
>         	PREVENT_LOOPING=true; export PREVENT_LOOPING
>                 test ! -x /bin/ksh || exec /bin/ksh "$0" "$@"
>                 test ! -x /bin/bash || exec /bin/bash "$0" "$@"
>         fi
>         echo "Cannot find compatible shell for '$0'" >&2
>         exit 1
> fi

Unfortunately, no. The /bin/sh says "bad substitution" and does not run
the script at all. I also tried

eval 'echo ${A%%1}'; echo ok

but that does not run the 'echo ok' and fails also.


I can see three possible solutions

1) use bash or ksh in the shebang line
2) rewrite the script as I gave the overview
3) declare that solaris 10 /bin/sh is not compatible with configure
script


Frankly even 3) is viable option, one just have to remember to run
'bash configure'. If everything else would work, I would be happy :)

Thank you
-- 
	Vlad

Re: [PATCH 1/4] Make configure use /bin/bash instead of /bin/sh

[Tomi Ollila]

On Wed, Apr 11 2012, Vladimir Marek <Vladimir.Marek@Oracle.COM> wrote:

> Hi,
>

[ ... ]

>
>> Does the configure script work if you replace /bin/sh with /bin/ksh
>> in your Solaris box
>
> yes, it does work if executed by /bin/bash or /bin/ksh
>
>
>> If yes, something like the following could be added to the beginning
>> of 'configure'
>> 
>> option=option=value
>> if test ! x"${option$*=}" = x"value" 2>/dev/null; then
>> 	if test x"${PREVENT_LOOPING-}" = x; then
>>         	PREVENT_LOOPING=true; export PREVENT_LOOPING
>>                 test ! -x /bin/ksh || exec /bin/ksh "$0" "$@"
>>                 test ! -x /bin/bash || exec /bin/bash "$0" "$@"
>>         fi
>>         echo "Cannot find compatible shell for '$0'" >&2
>>         exit 1
>> fi
>
> Unfortunately, no. The /bin/sh says "bad substitution" and does not run
> the script at all. I also tried
>
> eval 'echo ${A%%1}'; echo ok
>
> but that does not run the 'echo ok' and fails also.

You're right! I tested this stuff using heirloom-sh
from http://heirloom.sourceforge.net/sh.html

It is interesting that the shell stops executing when 
it finds this syntax (instead of contnuing, even without -e)

> I can see three possible solutions
>
> 1) use bash or ksh in the shebang line

Cannot do there are systems lacking /bin/bash & /bin/ksh

> 2) rewrite the script as I gave the overview

Some work todo; case construct can do option key matching to get 
identical interface and then cut or sed to get just option value.

> 3) declare that solaris 10 /bin/sh is not compatible with configure
> script
>
> Frankly even 3) is viable option, one just have to remember to run
> 'bash configure'. If everything else would work, I would be happy :)

Option 4) use the following heuristics:

case ~ in '~')
	if test x"${PREVENT_LOOPING-}" = x; then
		PREVENT_LOOPING=true; export PREVENT_LOOPING
		for x in /bin/ksh /bin/bash /usr/bin/bash
		do test ! -x "$x" || exec "$x" "$0" "$@"
                done
	fi
	echo "Cannot find compatible shell for '$0'" >&2
	exit 1
esac

i.e. if tilde expansion is not done guess this shell is not 
compatible enough

Option 5) do substitution check in subshell:

( option=option=value; : ${option$*=} ) 2>/dev/null || {
	if test x"${PREVENT_LOOPING-}" = x; then
		PREVENT_LOOPING=true; export PREVENT_LOOPING
		for x in /bin/ksh /bin/bash /usr/bin/bash
		do test ! -x "$x" || exec "$x" "$0" "$@"
                done
	fi
	echo "Cannot find compatible shell for '$0'" >&2
	exit 1
}

>
> Thank you
> -- 
> 	Vlad

Tomi

Re: [PATCH 2/4] dirent->d_type not available on Soalris

[Tomi Ollila]

On Mon, Apr 09 2012, Vladimir.Marek@oracle.com wrote:

> From: Vladimir Marek <vlmarek@volny.cz>
>
> The inspiration was taken from similar issue in mutt:
> http://does-not-exist.org/mail-archives/mutt-dev/msg11290.html
>
> Signed-off-by: Vladimir Marek <vlmarek@volny.cz>
> ---

Code looks pretty good, but 2 issues

1) Commit message should contain more verbose information what and 
   why something was done.

2) Does these #ifdefs break code indenters such as uncrustify(1),
   indent(1) and emacs(1) indent functionality.
   (That used to happen but maybe these indenters are smarted today.

Tomi

>  notmuch-new.c |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)

Re: [PATCH 2/4] dirent->d_type not available on Soalris

[Austin Clements]

Correct me if I'm mistaken, but d_name will only be a basename, so
your calls to stat will fail for files that are not in the current
directory.  I think in all of the situations you had to call stat, we
already construct the absolute path of the file (sometimes a little
later in the code, but it can easily be moved), so this should be easy
to fix.

Rather than sprinkling portability code throughout notmuch-new, it
seems like it would be simpler if this logic were wrapped in a
separate function.  For example, something along the (completely
untested) lines of,

static mode_t
dirent_type (const struct *entry, const char *abspath)
{
    struct stat statbuf;
#ifdef _DIRENT_HAVE_D_TYPE
    static const mode_t modes[] = {
	[DT_BLK]  = S_IFBLK,
	[DT_CHR]  = S_IFCHR,
	[DT_DIR]  = S_IFDIR,
	[DT_FIFO] = S_IFIFO,
	[DT_LNK]  = S_IFLNK,
	[DT_REG]  = S_IFREG,
	[DT_SOCK] = S_IFSOCK
    };
    if (entry->d_type >= 0 && entry->d_type < sizeof(modes)/sizeof(modes[0]) &&
	modes[entry->d_type])
	return modes[entry->d_type];
#endif

    if (stat(abspath, &statbuf) == -1)
	return -1;
    return statbuf.st_mode & S_IFMT;
}

This has the added benefit of correctly handling DT_UNKNOWN, which we
currently don't.

Instead of taking the absolute path of the file, this could take the
absolute path of the containing directory and construct the full path
from that and d_name; that would probably be a nicer interface, but it
would be redundant computation.

Quoth Vladimir.Marek@oracle.com on Apr 09 at  6:32 pm:
> From: Vladimir Marek <vlmarek@volny.cz>
> 
> The inspiration was taken from similar issue in mutt:
> http://does-not-exist.org/mail-archives/mutt-dev/msg11290.html
> 
> Signed-off-by: Vladimir Marek <vlmarek@volny.cz>
> ---
>  notmuch-new.c |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)
> 
> diff --git a/notmuch-new.c b/notmuch-new.c
> index 4f13535..3d265bd 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -21,6 +21,9 @@
>  #include "notmuch-client.h"
>  
>  #include <unistd.h>
> +#ifndef _DIRENT_HAVE_D_TYPE
> +#include <sys/types.h>
> +#endif
>  
>  typedef struct _filename_node {
>      char *filename;
> @@ -167,7 +170,14 @@ _entries_resemble_maildir (struct dirent **entries, int count)
>      int i, found = 0;
>  
>      for (i = 0; i < count; i++) {
> +#ifdef _DIRENT_HAVE_D_TYPE
>  	if (entries[i]->d_type != DT_DIR && entries[i]->d_type != DT_UNKNOWN)
> +#else
> +	struct stat statbuf;
> +	if (stat(entries[i]->d_name, &statbuf) == -1)
> +		continue;
> +	if (! S_ISDIR(statbuf.st_mode))
> +#endif
>  	    continue;
>  
>  	if (strcmp(entries[i]->d_name, "new") == 0 ||
> @@ -258,6 +268,9 @@ add_files_recursive (notmuch_database_t *notmuch,
>      struct stat st;
>      notmuch_bool_t is_maildir, new_directory;
>      const char **tag;
> +#ifndef _DIRENT_HAVE_D_TYPE
> +    struct stat statbuf;
> +#endif
>  
>      if (stat (path, &st)) {
>  	fprintf (stderr, "Error reading directory %s: %s\n",
> @@ -328,9 +341,16 @@ add_files_recursive (notmuch_database_t *notmuch,
>  	 * scandir results, then it might be a directory (and if not,
>  	 * then we'll stat and return immediately in the next level of
>  	 * recursion). */
> +#ifdef _DIRENT_HAVE_D_TYPE
>  	if (entry->d_type != DT_DIR &&
>  	    entry->d_type != DT_LNK &&
>  	    entry->d_type != DT_UNKNOWN)
> +#else
> +	if (stat(entry->d_name, &statbuf) == -1)
> +		continue;
> +	if (!(statbuf.st_mode & S_IFDIR) &&
> +	    !(statbuf.st_mode & S_IFLNK))
> +#endif
>  	{
>  	    continue;
>  	}
> @@ -427,7 +447,11 @@ add_files_recursive (notmuch_database_t *notmuch,
>  	 *
>  	 * In either case, a stat does the trick.
>  	 */
> +#ifdef _DIRENT_HAVE_D_TYPE
>  	if (entry->d_type == DT_LNK || entry->d_type == DT_UNKNOWN) {
> +#else
> +	if (stat(entry->d_name, &statbuf) == -1 || statbuf.st_mode & S_IFLNK) {
> +#endif
>  	    int err;
>  
>  	    next = talloc_asprintf (notmuch, "%s/%s", path, entry->d_name);
> @@ -443,7 +467,11 @@ add_files_recursive (notmuch_database_t *notmuch,
>  
>  	    if (! S_ISREG (st.st_mode))
>  		continue;
> +#ifdef _DIRENT_HAVE_D_TYPE
>  	} else if (entry->d_type != DT_REG) {
> +#else
> +	} else if (statbuf.st_mode & S_IFREG) {
> +#endif
>  	    continue;
>  	}
>  

Re: [PATCH 4/4] Explicitly type void* pointers

[Austin Clements]

On Mon, 09 Apr 2012, Jani Nikula <jani@nikula.org> wrote:
> Vladimir Marek <Vladimir.Marek@Oracle.COM> writes:
> I'm throwing in a third alternative below. Does it work for you? I think
> it's both prettier and uglier than the above at the same time! ;)
>
> A middle ground would be to change the callers to use
> "notmuch_talloc_steal", and just #define notmuch_talloc_steal
> talloc_steal if __GNUC__ >= 3.
>
> One could argue upstream talloc should have this, but OTOH it's a C
> library.
>
> BR,
> Jani.
>
>
> diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
> index ea836f7..83b46e8 100644
> --- a/lib/notmuch-private.h
> +++ b/lib/notmuch-private.h
> @@ -499,4 +499,22 @@ _notmuch_filenames_create (const void *ctx,
>  
>  NOTMUCH_END_DECLS
>  
> +#ifdef __cplusplus
> +/* Implicit typecast from 'void *' to 'T *' is okay in C, but not in
> + * C++. In talloc_steal, an explicit cast is provided for type safety
> + * in some GCC versions. Otherwise, a cast is required. Provide a
> + * template function for this to maintain type safety, and redefine
> + * talloc_steal to use it.
> + */
> +#if !(__GNUC__ >= 3)
> +template <class T>
> +T *notmuch_talloc_steal(const void *new_ctx, const T *ptr)
> +{
> +    return static_cast<T*>(talloc_steal(new_ctx, ptr));
> +}
> +#undef talloc_steal
> +#define talloc_steal notmuch_talloc_steal
> +#endif
> +#endif
> +
>  #endif

This looks good to me.  I was originally concerned that this depended on
talloc_steal being a macro, but I realized that's not actually the case.
Care to roll a real patch?

Re: [PATCH 4/4] Explicitly type void* pointers

[Jani Nikula]

On Wed, 11 Apr 2012 17:11:13 -0400, Austin Clements <amdragon@MIT.EDU> wrote:
> On Mon, 09 Apr 2012, Jani Nikula <jani@nikula.org> wrote:
> > Vladimir Marek <Vladimir.Marek@Oracle.COM> writes:
> > I'm throwing in a third alternative below. Does it work for you? I think
> > it's both prettier and uglier than the above at the same time! ;)
> >
> > A middle ground would be to change the callers to use
> > "notmuch_talloc_steal", and just #define notmuch_talloc_steal
> > talloc_steal if __GNUC__ >= 3.
> >
> > One could argue upstream talloc should have this, but OTOH it's a C
> > library.
> >
> > BR,
> > Jani.
> >
> >
> > diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
> > index ea836f7..83b46e8 100644
> > --- a/lib/notmuch-private.h
> > +++ b/lib/notmuch-private.h
> > @@ -499,4 +499,22 @@ _notmuch_filenames_create (const void *ctx,
> >  
> >  NOTMUCH_END_DECLS
> >  
> > +#ifdef __cplusplus
> > +/* Implicit typecast from 'void *' to 'T *' is okay in C, but not in
> > + * C++. In talloc_steal, an explicit cast is provided for type safety
> > + * in some GCC versions. Otherwise, a cast is required. Provide a
> > + * template function for this to maintain type safety, and redefine
> > + * talloc_steal to use it.
> > + */
> > +#if !(__GNUC__ >= 3)
> > +template <class T>
> > +T *notmuch_talloc_steal(const void *new_ctx, const T *ptr)
> > +{
> > +    return static_cast<T*>(talloc_steal(new_ctx, ptr));
> > +}
> > +#undef talloc_steal
> > +#define talloc_steal notmuch_talloc_steal
> > +#endif
> > +#endif
> > +
> >  #endif
> 
> This looks good to me.  I was originally concerned that this depended on
> talloc_steal being a macro, but I realized that's not actually the case.
> Care to roll a real patch?

Sure.

One question: the template must be outside NOTMUCH_{BEGIN,END}_DECLS
(which are just macros for extern "C" block) but should it be within the
#pragma GCC visibility push(hidden) and pop directives? I'm not familiar
with that.

Thanks for the review.


BR,
Jani.

Re: [PATCH 4/4] Explicitly type void* pointers

[Austin Clements]

Quoth Jani Nikula on Apr 12 at  8:02 am:
> On Wed, 11 Apr 2012 17:11:13 -0400, Austin Clements <amdragon@MIT.EDU> wrote:
> > On Mon, 09 Apr 2012, Jani Nikula <jani@nikula.org> wrote:
> > > Vladimir Marek <Vladimir.Marek@Oracle.COM> writes:
> > > I'm throwing in a third alternative below. Does it work for you? I think
> > > it's both prettier and uglier than the above at the same time! ;)
> > >
> > > A middle ground would be to change the callers to use
> > > "notmuch_talloc_steal", and just #define notmuch_talloc_steal
> > > talloc_steal if __GNUC__ >= 3.
> > >
> > > One could argue upstream talloc should have this, but OTOH it's a C
> > > library.
> > >
> > > BR,
> > > Jani.
> > >
> > >
> > > diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
> > > index ea836f7..83b46e8 100644
> > > --- a/lib/notmuch-private.h
> > > +++ b/lib/notmuch-private.h
> > > @@ -499,4 +499,22 @@ _notmuch_filenames_create (const void *ctx,
> > >  
> > >  NOTMUCH_END_DECLS
> > >  
> > > +#ifdef __cplusplus
> > > +/* Implicit typecast from 'void *' to 'T *' is okay in C, but not in
> > > + * C++. In talloc_steal, an explicit cast is provided for type safety
> > > + * in some GCC versions. Otherwise, a cast is required. Provide a
> > > + * template function for this to maintain type safety, and redefine
> > > + * talloc_steal to use it.
> > > + */
> > > +#if !(__GNUC__ >= 3)
> > > +template <class T>
> > > +T *notmuch_talloc_steal(const void *new_ctx, const T *ptr)
> > > +{
> > > +    return static_cast<T*>(talloc_steal(new_ctx, ptr));
> > > +}
> > > +#undef talloc_steal
> > > +#define talloc_steal notmuch_talloc_steal
> > > +#endif
> > > +#endif
> > > +
> > >  #endif
> > 
> > This looks good to me.  I was originally concerned that this depended on
> > talloc_steal being a macro, but I realized that's not actually the case.
> > Care to roll a real patch?
> 
> Sure.
> 
> One question: the template must be outside NOTMUCH_{BEGIN,END}_DECLS
> (which are just macros for extern "C" block) but should it be within the
> #pragma GCC visibility push(hidden) and pop directives? I'm not familiar
> with that.

Hmm.  I'm not sure, but it can't hurt to have it inside the visibility
directives.  My guess would be that it will affect the visibility of
the symbols produced when the template is instantiated, which we don't
want to be visible outside the library.  (As long as that doesn't mess
up the linker's handling of weak symbols.)

> Thanks for the review.
> 
> 
> BR,
> Jani.

Re: [PATCH 4/4] Explicitly type void* pointers

[Jani Nikula]

I tagged this patch notmuch::obsolete (see [1]) as the template
workaround was pushed to master (commit
de0557477d908be26615e8fda9f5eb62bed68b65).

Jani.


[1] http://nmbug.tethera.net/status/


On Mon, 09 Apr 2012, Vladimir.Marek@oracle.com wrote:
> From: Vladimir Marek <vlmarek@volny.cz>
>
>
> Signed-off-by: Vladimir Marek <vlmarek@volny.cz>
> ---
>  lib/database.cc |    2 +-
>  lib/message.cc  |    2 +-
>  lib/thread.cc   |    2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/lib/database.cc b/lib/database.cc
> index 16c4354..3c82632 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -1361,7 +1361,7 @@ _resolve_message_id_to_thread_id (notmuch_database_t *notmuch,
>  	return status;
>  
>      if (message) {
> -	*thread_id_ret = talloc_steal (ctx,
> +	*thread_id_ret = (const char*)talloc_steal (ctx,
>  				       notmuch_message_get_thread_id (message));
>  
>  	notmuch_message_destroy (message);
> diff --git a/lib/message.cc b/lib/message.cc
> index 0075425..d56d442 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -220,7 +220,7 @@ _notmuch_message_create_for_message_id (notmuch_database_t *notmuch,
>  									    message_id,
>  									    &message);
>      if (message)
> -	return talloc_steal (notmuch, message);
> +	return (notmuch_message_t*) talloc_steal (notmuch, message);
>      else if (*status_ret)
>  	return NULL;
>  
> diff --git a/lib/thread.cc b/lib/thread.cc
> index e976d64..d41ff3e 100644
> --- a/lib/thread.cc
> +++ b/lib/thread.cc
> @@ -225,7 +225,7 @@ _thread_add_message (notmuch_thread_t *thread,
>      char *clean_author;
>  
>      _notmuch_message_list_add_message (thread->message_list,
> -				       talloc_steal (thread, message));
> +				       (_notmuch_message*)talloc_steal (thread, message));
>      thread->total_messages++;
>  
>      g_hash_table_insert (thread->message_hash,
> -- 
> 1.7.3.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/4] Make configure use /bin/bash instead of /bin/sh

[David Bremner]

Vladimir.Marek@oracle.com writes:

> diff --git a/configure b/configure
> index 71981b7..6870341 100755
> --- a/configure
> +++ b/configure
> @@ -1,4 +1,4 @@
> -#! /bin/sh
> +#! /bin/bash

I realize the true story is more complicated than this, but how do
people feel about specifying bash for configure until/unless we get a
nicer solution?

d

Re: [PATCH 1/4] Make configure use /bin/bash instead of /bin/sh

[Tomi Ollila]

On Mon, Apr 30 2012, David Bremner <david@tethera.net> wrote:

> Vladimir.Marek@oracle.com writes:
>
>> diff --git a/configure b/configure
>> index 71981b7..6870341 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1,4 +1,4 @@
>> -#! /bin/sh
>> +#! /bin/bash
>
> I realize the true story is more complicated than this, but how do
> people feel about specifying bash for configure until/unless we get a
> nicer solution?

if /bin/sh is not posix-capable, it is more probable that bash is called
/usr/local/bin/bash than /bin/bash

I've submitted this

id:"1334589199-25894-1-git-send-email-tomi.ollila@iki.fi"

(which, of course, does not check /usr/local/bin/bash ;/)

There is also alternative, more heuristic approach, mentioned
as option 4 in 

id:"m2wr5mm7gb.fsf@guru.guru-group.fi"

> d

Tomi

Re: [PATCH 3/4] Private strsep implementation

[Ethan Glasser-Camp]

Vladimir.Marek@oracle.com writes:

> From: Vladimir Marek <vlmarek@volny.cz>
>
> strsep is not available on Solaris 10, so we stole the one used by
> mutt.

Hi! Just going through the patch queue. This patch looks fine to me, but
it no longer applies cleanly to master. Can you rebase it? It'll have my
+1.

Ethan