Re: Comments on xerces-c patch please
On Thu, 2011-08-04 at 19:10 +0200, Guillem Jover wrote:
> Hi!
>
> On Thu, 2011-08-04 at 15:05:34 +0200, Svante Signell wrote:
> > On Thu, 2011-08-04 at 13:39 +0200, Guillem Jover wrote:
> > > Also given that realpath(path, NULL) is in POSIX.1-2008 and has been
> > > supported for a long time on GNU/* and FreeBSD systems I'd change the
> > > __GNU__ check to a configure check for the realpath functionality.
>
> > There is already a check for realpath in configure.ac. But using that
> > will exclude the else part, see below:
...
> I guess I was not clear enough, here what we want to check for is if
> “realpath(path, NULL)” works correctly (notice the NULL argument),
> before POSIX.1-2008 (and newer GNU/* and FreeBSD systems) this was
> undefined behaviour, so even if the system has realpath() it might
> not have what we want from it.
Why didn't you update the patch yourself instead of complaining on my
patch, or at least give a link to working code. Currently I don't know
how to write this test, and after your reply I don't know either.
Constructive criticism is always better than destructive.
> > Same here: getcwd is checked for in both if and else parts.
>
> Here I was also referring specifically to the behaviour of
> “getcwd(NULL, 0)” (notice the NULL and 0 arguments).
See above.
> > Should I make it unconditional then? Then probably the DM complains the
> > the patch changes the original program code for other archs.
>
> Making it unconditional lowers the chances of this being accepted
> upstream, because it reduces the system this will work on. If upstream
> is fine limiting their project to GNU/*, FreeBSD and POSIX.1-2008
> systems, then that'd be ok, but you'd have to ask upstream and you
> should not assume such things by default.
Ideally the main interface is DMs and they communicate with upstream.
Sometimes, however, communicating with upstream directly leads to a
faster solution.
> For the indentation (spaces/tabs) I'd recommend you enable something
> like this in your editor, vim in this case but emacs and other editors
> should have an equivalent:
>
> let c_space_errors=1
> set listchars=tab:»‐,trail:·,extends:…,nbsp:‗
> set list
This is constructive, but unfortunately I don't use vim and have no idea
what this means in Emacs terms.
> For the blank lines, coding style etc, you should review your own
> patches before sending them.
Why having reviews at all then, if everything is perfect by definition.
No more updated patch from me this time, maybe you can take over and
make it perfect. I will consider if I'm going to continue struggling
with GNU/Hurd. There are better ways to spend your _free_ _unpaid_ time,
instead of being criticised when trying to contribute.
How many patches per day are sent by the very few GNU/Hurd developers?
After following the lists and #hurd for some time now my impression is
that most communication is words mainly, not many lines of code are
produced.
Bye :-(
Reply to: