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

[Not Zed <notzed ximian com>]

On Fri, 2005-05-20 at 15:46 +0300, Tor Lillqvist wrote:
> 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?

Well none of the places in my code use G_LOCK_DEFINE_STATIC, so by that
argument your change is unecessary.

Anyway there is a good reason not to, incase you want to change the lock
type, i.e. to a recursive lock.  You only need to change one line not n
lines.

>  > 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.

>From the patch it just looked like lines with a tab on them, which Jeff
always does.  Anyway like i said the changes in this patch are ok.

>  > -#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.)

gtk does a lot of forward references and stuff now, so it is designed to
be included in parts.

*shrug* maybe glib doesn't matter, but why change it if you dont need
to?

> 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?

I guess it should be removed then.

>  > 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?)

*shrug* maybe it was some other function i was thinking of, or maybe it
was changed.

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

You should be able to write a better implementation in windows anyway,
using pipes and cond-waits and other crap is pretty messy way to
implement a message port.

>  > 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_*).

Well thats most of the guts, the other stuff is just portable utils
which dont belong in that file anyway.

But sure whatever, its not worth arguing about.

> (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

How strangely busted.

> 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?)

Well it would be better to canonicalise on / path names entirely, yes,
in which case that change wouldn't be required at all.

But as soon as you let any \'s sneak in the code is going to be littered
with g_path_separator crap anyway.