Re: Comments on xerces-c patch please

[Svante Signell <svante.signell@telia.com>]

On Thu, 2011-08-04 at 13:39 +0200, Guillem Jover wrote:

> Well, you didn't give much time for feedback. Anyway here it goes:
> 
> > --- xerces-c-3.1.1/src/xercesc/util/FileManagers/PosixFileMgr.cpp.orig	2008-07-04 09:23:56.000000000 +0000
> > +++ xerces-c-3.1.1/src/xercesc/util/FileManagers/PosixFileMgr.cpp	2011-08-03 21:58:23.000000000 +0000
> > @@ -187,6 +187,17 @@
> >      ArrayJanitor<char> janText(newSrc, manager);
> >  
> >      // Use a local buffer that is big enough for the largest legal path
> > +#ifdef __GNU__
> > +    char *absPath;
> > +    XMLCh *ret;
> > +    // get the absolute path
> > +    absPath=realpath(newSrc, NULL);
> > +    if (absPath == NULL)
> > +   		ThrowXMLwithMemMgr(XMLPlatformUtilsException, XMLExcepts::File_CouldNotGetBasePathName, manager);
> > +    ret = XMLString::transcode(absPath, manager);
> > +    free (absPath);
> > +    return ret; 
> > +#else
> 
> Follow the existing coding style: add a blank line between the
> variable definintions and the actual code block, add missing spaces
> around =, remove trailing space after “return ret; ”.
> 
> 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.
> 
> >      char absPath[PATH_MAX + 1];
> >      
> >      // get the absolute path
> > @@ -194,20 +205,34 @@
> >     		ThrowXMLwithMemMgr(XMLPlatformUtilsException, XMLExcepts::File_CouldNotGetBasePathName, manager);
> >  
> >      return XMLString::transcode(absPath, manager);
> > +#endif
> >  }

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


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

> >      char dirBuf[PATH_MAX + 2];
> >      char *curDir = getcwd(&dirBuf[0], PATH_MAX + 1);
> > -
> 
> Do not change unrelated spaces and blank lines for other code blocks.
> 
> >      if (!curDir)
> >          ThrowXMLwithMemMgr(XMLPlatformUtilsException,
> >                   XMLExcepts::File_CouldNotGetBasePathName, manager);
> >  
> >      return XMLString::transcode(curDir, manager);
> > +#endif
> >  }

Same here: getcwd is checked for in both if and else parts.

Should I make it unconditional then? Then probably the DM complains the
the patch changes the original program code for other archs.