Re: Comments on xerces-c patch please

[Guillem Jover <guillem@debian.org>]

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:
> #else
>     char absPath[PATH_MAX + 1];
> 
>     // get the absolute
> path                                                    
>     if (!realpath(newSrc, absPath))
>                 ThrowXMLwithMemMgr(XMLPlatformUtilsException,
> XMLExcepts::File_\
> CouldNotGetBasePathName, manager);
> 
>     return XMLString::transcode(absPath, manager);
> #endif

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.

> > >  XMLCh*
> > >  PosixFileMgr::getCurrentDirectory(MemoryManager* const manager)
> > >  {
> > > +#ifdef __GNU__
> > > +    char *dirBuf=NULL;
> > > +    XMLCh *ret;
> > > +    size_t dummy = 0;
> > > +    char *curDir = getcwd(dirBuf, dummy);
> > > +    if (curDir == NULL)
> > > +        ThrowXMLwithMemMgr(XMLPlatformUtilsException,
> > > +                 XMLExcepts::File_CouldNotGetBasePathName, manager);
> > > +
> > > +    ret = XMLString::transcode(curDir, manager);
> > > +    free (curDir);
> > > +    return ret;
> > > +#else
> > 
> > There's no need for the dummy or dirBuf variables. The same style
> > comments as the above. And getcwd(NULL, 0) has also been supported on
> > FreeBSD and GNU/* for a long time, it would be better to check for its
> > availabilty at configure time and use that whenever possible.
> 
> OK, changed!

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

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

Regarding Debian maintainers complaining about patches, you have
mentioned this several times now on patch reviews, and I'm not sure
where this sentiment is coming from. The patches should foremost be
correct and not introduce regressions (which includes portability),
they should flow with the rest of the code style and design, each
logical change should come in a different patch (although some
upstreams do not care about this/do not mind, but you should always
default to the safest option), the changes should be as minimal as
possible, but sometimes it might make sense to refactor the code
before introducing your changes. If these "rules" are followed you'll
have the best chance of getting your changes accepted. You should also
always consider the patches you are sending might (and ideally should)
end up upstream, and as such should follow the same considerations as
if they were being sent there directly.

And in the end it's not really about making things conditional or not
to avoid disturbing the main code paths due to support for a specific
system, because in most cases adding the additional code branches
makes it more difficult to maintain and the code becomes susceptible
to bit rot, but in some other cases adding a conditional code branch
is the right solution, so it always depends on the context.

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

For the blank lines, coding style etc, you should review your own
patches before sending them.

regards,
guillem