* [PATCH 1/4] Make configure use /bin/bash instead of /bin/sh
2012-04-09 10:17 notmuch on Solaris Vladimir.Marek
@ 2012-04-09 10:17 ` Vladimir.Marek
2012-04-09 11:10 ` Jani Nikula
2012-04-30 11:58 ` David Bremner
2012-04-09 10:17 ` [PATCH 2/4] dirent->d_type not available on Soalris Vladimir.Marek
` (2 subsequent siblings)
3 siblings, 2 replies; 27+ messages in thread
From: Vladimir.Marek @ 2012-04-09 10:17 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 1/4] Make configure use /bin/bash instead of /bin/sh
2012-04-09 10:17 ` [PATCH 1/4] Make configure use /bin/bash instead of /bin/sh Vladimir.Marek
@ 2012-04-09 11:10 ` Jani Nikula
2012-04-09 12:19 ` Vladimir Marek
2012-04-30 11:58 ` David Bremner
1 sibling, 1 reply; 27+ messages in thread
From: Jani Nikula @ 2012-04-09 11:10 UTC (permalink / raw)
To: Vladimir.Marek; +Cc: Notmuch Mail, Vladimir Marek
[-- 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 --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/4] Make configure use /bin/bash instead of /bin/sh
2012-04-09 11:10 ` Jani Nikula
@ 2012-04-09 12:19 ` Vladimir Marek
2012-04-10 17:26 ` Tomi Ollila
0 siblings, 1 reply; 27+ messages in thread
From: Vladimir Marek @ 2012-04-09 12:19 UTC (permalink / raw)
To: Notmuch Mail
> > 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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/4] Make configure use /bin/bash instead of /bin/sh
2012-04-09 12:19 ` Vladimir Marek
@ 2012-04-10 17:26 ` Tomi Ollila
2012-04-11 8:43 ` Vladimir Marek
0 siblings, 1 reply; 27+ messages in thread
From: Tomi Ollila @ 2012-04-10 17:26 UTC (permalink / raw)
To: Vladimir Marek, Notmuch Mail
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/4] Make configure use /bin/bash instead of /bin/sh
2012-04-10 17:26 ` Tomi Ollila
@ 2012-04-11 8:43 ` Vladimir Marek
2012-04-11 18:47 ` Tomi Ollila
0 siblings, 1 reply; 27+ messages in thread
From: Vladimir Marek @ 2012-04-11 8:43 UTC (permalink / raw)
To: Notmuch Mail
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/4] Make configure use /bin/bash instead of /bin/sh
2012-04-11 8:43 ` Vladimir Marek
@ 2012-04-11 18:47 ` Tomi Ollila
0 siblings, 0 replies; 27+ messages in thread
From: Tomi Ollila @ 2012-04-11 18:47 UTC (permalink / raw)
To: Vladimir Marek, Notmuch Mail
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/4] Make configure use /bin/bash instead of /bin/sh
2012-04-09 10:17 ` [PATCH 1/4] Make configure use /bin/bash instead of /bin/sh Vladimir.Marek
2012-04-09 11:10 ` Jani Nikula
@ 2012-04-30 11:58 ` David Bremner
2012-04-30 12:09 ` Tomi Ollila
1 sibling, 1 reply; 27+ messages in thread
From: David Bremner @ 2012-04-30 11:58 UTC (permalink / raw)
To: Vladimir.Marek, notmuch; +Cc: Vladimir Marek
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/4] Make configure use /bin/bash instead of /bin/sh
2012-04-30 11:58 ` David Bremner
@ 2012-04-30 12:09 ` Tomi Ollila
0 siblings, 0 replies; 27+ messages in thread
From: Tomi Ollila @ 2012-04-30 12:09 UTC (permalink / raw)
To: David Bremner, Vladimir.Marek, notmuch; +Cc: Vladimir Marek
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 2/4] dirent->d_type not available on Soalris
2012-04-09 10:17 notmuch on Solaris Vladimir.Marek
2012-04-09 10:17 ` [PATCH 1/4] Make configure use /bin/bash instead of /bin/sh Vladimir.Marek
@ 2012-04-09 10:17 ` Vladimir.Marek
2012-04-09 11:17 ` Jani Nikula
2012-04-09 10:17 ` [PATCH 3/4] Private strsep implementation Vladimir.Marek
2012-04-09 10:17 ` [PATCH 4/4] Explicitly type void* pointers Vladimir.Marek
3 siblings, 1 reply; 27+ messages in thread
From: Vladimir.Marek @ 2012-04-09 10:17 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 2/4] dirent->d_type not available on Soalris
2012-04-09 10:17 ` [PATCH 2/4] dirent->d_type not available on Soalris Vladimir.Marek
@ 2012-04-09 11:17 ` Jani Nikula
2012-04-09 15:12 ` Vladimir Marek
0 siblings, 1 reply; 27+ messages in thread
From: Jani Nikula @ 2012-04-09 11:17 UTC (permalink / raw)
To: Vladimir.Marek; +Cc: Notmuch Mail, Vladimir Marek
[-- 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 --]
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 2/4] dirent->d_type not available on Soalris
2012-04-09 11:17 ` Jani Nikula
@ 2012-04-09 15:12 ` Vladimir Marek
2012-04-09 15:46 ` Adam Wolfe Gordon
0 siblings, 1 reply; 27+ messages in thread
From: Vladimir Marek @ 2012-04-09 15:12 UTC (permalink / raw)
To: Notmuch Mail
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/4] dirent->d_type not available on Soalris
2012-04-09 15:12 ` Vladimir Marek
@ 2012-04-09 15:46 ` Adam Wolfe Gordon
2012-04-09 16:32 ` Vladimir.Marek
0 siblings, 1 reply; 27+ messages in thread
From: Adam Wolfe Gordon @ 2012-04-09 15:46 UTC (permalink / raw)
To: Notmuch Mail
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 2/4] dirent->d_type not available on Soalris
2012-04-09 15:46 ` Adam Wolfe Gordon
@ 2012-04-09 16:32 ` Vladimir.Marek
2012-04-11 18:57 ` Tomi Ollila
2012-04-11 19:36 ` Austin Clements
0 siblings, 2 replies; 27+ messages in thread
From: Vladimir.Marek @ 2012-04-09 16:32 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 2/4] dirent->d_type not available on Soalris
2012-04-09 16:32 ` Vladimir.Marek
@ 2012-04-11 18:57 ` Tomi Ollila
2012-04-11 19:36 ` Austin Clements
1 sibling, 0 replies; 27+ messages in thread
From: Tomi Ollila @ 2012-04-11 18:57 UTC (permalink / raw)
To: Vladimir.Marek, notmuch; +Cc: Vladimir Marek
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(-)
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/4] dirent->d_type not available on Soalris
2012-04-09 16:32 ` Vladimir.Marek
2012-04-11 18:57 ` Tomi Ollila
@ 2012-04-11 19:36 ` Austin Clements
1 sibling, 0 replies; 27+ messages in thread
From: Austin Clements @ 2012-04-11 19:36 UTC (permalink / raw)
To: Vladimir.Marek; +Cc: notmuch, Vladimir Marek
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;
> }
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 3/4] Private strsep implementation
2012-04-09 10:17 notmuch on Solaris Vladimir.Marek
2012-04-09 10:17 ` [PATCH 1/4] Make configure use /bin/bash instead of /bin/sh Vladimir.Marek
2012-04-09 10:17 ` [PATCH 2/4] dirent->d_type not available on Soalris Vladimir.Marek
@ 2012-04-09 10:17 ` Vladimir.Marek
2012-10-15 5:05 ` Ethan Glasser-Camp
2012-04-09 10:17 ` [PATCH 4/4] Explicitly type void* pointers Vladimir.Marek
3 siblings, 1 reply; 27+ messages in thread
From: Vladimir.Marek @ 2012-04-09 10:17 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 4/4] Explicitly type void* pointers
2012-04-09 10:17 notmuch on Solaris Vladimir.Marek
` (2 preceding siblings ...)
2012-04-09 10:17 ` [PATCH 3/4] Private strsep implementation Vladimir.Marek
@ 2012-04-09 10:17 ` Vladimir.Marek
2012-04-09 11:04 ` Jani Nikula
2012-04-15 21:05 ` Jani Nikula
3 siblings, 2 replies; 27+ messages in thread
From: Vladimir.Marek @ 2012-04-09 10:17 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 4/4] Explicitly type void* pointers
2012-04-09 10:17 ` [PATCH 4/4] Explicitly type void* pointers Vladimir.Marek
@ 2012-04-09 11:04 ` Jani Nikula
2012-04-09 18:15 ` Vladimir Marek
2012-04-15 21:05 ` Jani Nikula
1 sibling, 1 reply; 27+ messages in thread
From: Jani Nikula @ 2012-04-09 11:04 UTC (permalink / raw)
To: Vladimir.Marek; +Cc: Notmuch Mail, Vladimir Marek
[-- 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 --]
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 4/4] Explicitly type void* pointers
2012-04-09 11:04 ` Jani Nikula
@ 2012-04-09 18:15 ` Vladimir Marek
2012-04-09 21:31 ` Jani Nikula
0 siblings, 1 reply; 27+ messages in thread
From: Vladimir Marek @ 2012-04-09 18:15 UTC (permalink / raw)
To: Notmuch Mail
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 4/4] Explicitly type void* pointers
2012-04-09 18:15 ` Vladimir Marek
@ 2012-04-09 21:31 ` Jani Nikula
2012-04-10 6:53 ` Vladimir Marek
2012-04-11 21:11 ` Austin Clements
0 siblings, 2 replies; 27+ messages in thread
From: Jani Nikula @ 2012-04-09 21:31 UTC (permalink / raw)
To: Vladimir Marek, Notmuch Mail
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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 4/4] Explicitly type void* pointers
2012-04-09 21:31 ` Jani Nikula
@ 2012-04-10 6:53 ` Vladimir Marek
2012-04-11 21:11 ` Austin Clements
1 sibling, 0 replies; 27+ messages in thread
From: Vladimir Marek @ 2012-04-10 6:53 UTC (permalink / raw)
To: Jani Nikula; +Cc: Notmuch Mail
[...]
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 4/4] Explicitly type void* pointers
2012-04-09 21:31 ` Jani Nikula
2012-04-10 6:53 ` Vladimir Marek
@ 2012-04-11 21:11 ` Austin Clements
2012-04-12 8:02 ` Jani Nikula
1 sibling, 1 reply; 27+ messages in thread
From: Austin Clements @ 2012-04-11 21:11 UTC (permalink / raw)
To: Jani Nikula, Vladimir Marek, Notmuch Mail
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?
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 4/4] Explicitly type void* pointers
2012-04-11 21:11 ` Austin Clements
@ 2012-04-12 8:02 ` Jani Nikula
2012-04-12 17:57 ` Austin Clements
0 siblings, 1 reply; 27+ messages in thread
From: Jani Nikula @ 2012-04-12 8:02 UTC (permalink / raw)
To: Austin Clements, Vladimir Marek, Notmuch Mail
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.
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 4/4] Explicitly type void* pointers
2012-04-12 8:02 ` Jani Nikula
@ 2012-04-12 17:57 ` Austin Clements
0 siblings, 0 replies; 27+ messages in thread
From: Austin Clements @ 2012-04-12 17:57 UTC (permalink / raw)
To: Jani Nikula; +Cc: Notmuch Mail
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 4/4] Explicitly type void* pointers
2012-04-09 10:17 ` [PATCH 4/4] Explicitly type void* pointers Vladimir.Marek
2012-04-09 11:04 ` Jani Nikula
@ 2012-04-15 21:05 ` Jani Nikula
1 sibling, 0 replies; 27+ messages in thread
From: Jani Nikula @ 2012-04-15 21:05 UTC (permalink / raw)
To: Vladimir.Marek, notmuch; +Cc: Vladimir Marek
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
^ permalink raw reply [flat|nested] 27+ messages in thread