Re: [xslt] Patch for xsl:sort lang support

[Roumen Petrov <bugtrack roumenpetrov info>]

Nick Wellnhofer wrote:
> Roumen Petrov wrote:
> > Roumen Petrov wrote:
> > > Daniel Veillard wrote:
> > > > On Tue, Jun 10, 2008 at 02:05:12AM +0200, Nick Wellnhofer wrote:
> > > > > Roumen Petrov wrote:
> > > > > > Nick Wellnhofer wrote:
> > > > > > > Roumen Petrov wrote:
> > > > > > > > Please find patch that is first attempt to avoid build problem 
> > > > > > > > after recent commits related to xsltlocale.
> > > > > > > >
> > > > > > > > I'm not convinced that xsltlocale.c is correct for msvcrt:
> > > > > > > > The manuals show that rfc3066 language tags are supported but 
> > > > > > > > the current code ignore all of them.
> > > > > > > I took the language codes from here:
> > > > > > > http://msdn.microsoft.com/en-us/library/39cwe7zf(VS.80).aspx
> > > > > > 10x. I see that the _create_locale is related to setlocale.
> > > > > Yes, _create_locale accepts the same parameters as setocale.
> > > > >
> > > > > > On this page 
> > > > > > http://msdn.microsoft.com/en-us/library/ms776260(VS.85).aspx
> > > > > > is listed another information but for other functions.
> > > > > > May be win32 locale has to be based on this.
> > > > > No, the CRT functions don't seem to accept those language codes.
> > > > >
> > > > > > > > Also I think that _create_locale is missing before msvcrt80. The 
> > > > > > > > proposed patch don't resolve this in makefile based win32 build.
> > > > > > > You're right, it seems it was introduced with VC8. Maybe it would 
> > > > > > > be best to have a configure switch to disable locale support in 
> > > > > > > case you want a build that runs without msvcr80.dll?
> > > > > > In the proposed patch configure will try detect presence of 
> > > > > > _create_locale if don't exist header xlocale.h , but not in 
> > > > > > makefile based build ( win32/Makefile.msvc ). May a new makefile 
> > > > > > win32/Makefile.msvcXX can ersolve problem.
> > > > > Your patch looks good. See the attached version (also against 
> > > > > current trunk) that additionally moves the defines to 
> > > > > libxslt/xsltconfig.h and adds a switch to win32/configure.js.
> > > > >
> > > > > I only wonder which setup uses GNU make and MSVCRT.
> > > > >
> > > > > > > > I wonder why the implementation assume that if exist xlocale.h 
> > > > > > > > the charmap is ascii.
> > > > > > > Which charmap do you mean?
> > > > > > EBCDIC
> > > > > I didn't think about that. Maybe one should add an appropriate 
> > > > > #ifdef somewhere...
> > > >
> > > >   Okay I applied and commited this patch which seems to integrate ideas
> > > > from both of you (i.e. i assume it superseeds Roumen's own 
> > > > libxslt-trunk-20080608.patch , right ?)
> > > yes, it is fine with me (only code review).
> > > (sorry for late reply but I'm too busy with other task and i don't 
> > > have time to test).
> > >
> > > >   Is there anything still pending on this ?
> > >
> > > I'm thinking about more backward compatible w32 implementation since 
> > > the current require msvcrt80.
> > > The xlocale based sort works fine.
> > > But the related bug can be closed.
> > >
> > > > Daniel
> > >
> > > Roumen
> >
> > Please find attached file "libxslt-trunk-20080625.diff.gz" with w32 
> > implementation based on winapi functions. It has to work on all 
> > NTs(nt4/w2k/xp/..). With some link quirks(not in the patch) it has to 
> > work on 9x(95/98/me).
> >
> > Roumen
>
> Looks very nice. I needed one little fix to get it to compile with the 
> Microsoft C compiler and nmake. See the attached patch (on top of your 
> patch) that also fixes two compiler warnings.
>
> We might also want to keep the following checks in xsltLocaleStrCmp
>
> if (str1 == str2) return(0);
> if (str1 == NULL) return(-1);
> if (str2 == NULL) return(1);
>
> so that it behaves like xmlStrcmp with regard to NULL pointers.
>
> Nick


Please find attached "libxslt-trunk-20080702.diff.gz" with suggested by 
Nick changes.

Roumen

Attachment: libxslt-trunk-20080702.diff.gz
Description: application/gzip