Re: [Evolution-hackers] e-d-s/libedataserver portability patch 1

[Tor Lillqvist <tml novell com>]

Thanks a lot for the constructive comments!

 > First look ... why change the code quoted below?  It works doens't it?
 > It seems like a patch just for the sake of changing the code style.

Well, consistency is an important part of good coding style, isn't it?
So why use 'static GStaticMutex something = G_STATIC_MUTEX_INIT' in
one place, and 'G_LOCK_DEFINE_STATIC(something)' in other places?

 > The same goes for changing blank lines - as much as i hate lines of
 > whitespace.  Do you have some sort of auto-formatting of existing code
 > in your editor? 

I use XEmacs, and many of the files do have the Mode: line at top, so
this shouldn't be a problem. If the file doesn't have a Mode: line, I
always manually set c-basic-offset to 8 when editing files that use
that style. I certainly haven't done any global reindentation or
spacing changes. Whitespace changes (except for the one mentioned
below) is unintentional.

 > (as much as it sounds anal, changing white-space is listed in the
 > patch guidelines as a no no).

Yeah, I definitely agree. But in this one file the amount of
whitespace was extreme (like, hundreds of empty spaces on blank
lines). Surely this is not on purpose? It makes the code look really
silly when the long empty lines even wrap. But if you insist, I'll
leave those lines alone.

 > -#include <glib/gfileutils.h>
 > -#include <glib/gmem.h>
 > -#include <glib/gmessages.h>
 > -#include <glib/gstrfuncs.h>
 > -#include <glib/gunicode.h>
 > -#include <glib/gutils.h>
 > -#include <glib/galloca.h>
 > -#include <glib/gconvert.h>
 > +#include <glib.h>
 > +#include <glib/gstdio.h>

 > Dont do this either, it was done like that on purpose ...

What purpose might that be? AFAIK, the recommended practice is to
incluse just <glib.h>, not to try to hand-pick which GLib headers one
might need. (Ditto for <gtk.h>.) And the GLib headers include each
others quite freely anyway, so it isn't like one would save anything
in compilation time. IMHO, including a selected subset of glib/*
headers is just misleading. (<glib/gstdio.h> is different, <glib.h>
doesn't include it on purpose.)

Furthermore, it isn't like #include optimization would have been much
of concern otherwise, many of the source files in e-d-s seem to have
largely copy-pasted #include blocks. One unnecessary #include in lots
of files is <sys/uio.h>, even though the files don't use
writev()/readv(). Or is there some magic in including <sys/uio.h> that
I am now aware of?

 > Note that g_file_test() is not the same as access().

Not in all cases, true. OK, I'll use g_access() instead of
g_file_test() here to minimize risk.

 > It will for example fail if a path component along the way isn't
 > readable or writable by the calling process, although it is
 > examinable.  Not a completely unlikely scenario.

Hmm, are you sure? For G_FILE_TEST_EXISTS, g_file_test() calls just
access(F_OK). For the other tests, it calls stat() (on the whole path)
and then checks the st_mode bits. Are there really situations where
access() and stat() would treat the permissions on *intermediate path
components* differently? (And if there are, is this system-dependent,
or actually specified in a standard?)

 > For the e-msgport stuff, can't you just write a new version for windows
 > and compile that instead?

If you insist, sure.

 > You're going to have to change almost every line of the code.

That's an overstatement. My current e-msgport.c differs in 143 lines
(if you count the "-" lines from diff -u), out of 1250 lines. Much of
it is boilerplate (pthread*->g_*).

(The "+" lines are a bit more numerous, as I had to add one new
function to create a "socketpair", to work as a select()able "pipe" on
Win32. Actually, that functionality probably should go into GLib.)

 > Also note that the cleanup stuff isn't needed - we never call
 > pthread_cancel because there were too many bugs in it when it was
 > originally written, and its almost impossible to write code to handle it
 > properly anyway.  So i'd say you can safely ignore/remove it.

Ah, cool. I would prefer if somebody who knows the code more closely
(and, most importantly, knows immediately how to test the affected
parts on Unix) would remove such dead code, though. Do you (or
somebody else) have the time to do this?

 > e_mutex could (should probably) be removed, if you change its uses to
 > the appropriate mutex types (recursive mutexes didn't exist in glib when
 > it was done).

OK, cool.

 > @@ -1,2 +1,7 @@
 > +if OS_WIN32
 > +NO_UNDEFINED = -no-undefined
 > +SOCKET_LIBS = -lws2_32
 > +endif

 > Can this be done at a higher-level?  e.g. in configure.in?  Otherwise
 > you're going to have to copy the if os_win32 stuff in every makefile.

True, will do that.

 > I'm not sure this is right either, do you really want to treat / as
 > the dir separator if there isn't any \'s?  Shouldn't you just use
 > G_DIR_SEPRATOR always?

/ and \ are equally valid on Win32 and must both be checked for. \ is
more canonical in the sense that if you ask for pathnames from Win32
(like getcwd()), you get backslashes. But OTOH, file: URIs use / only,
and many pathnames that whirl around inside Evo and the GNOME libs
take the form of URIs at some stage, don't they?

(Also, isn't it good if in source files where pathnames are
constructed one doesn't have to change those lines, but can keep the
"Unix" slashes as such, instead of having to sprinkle G_PATH_SEPARATOR
all over the place?)

Cheers,
--tml