1) Use C89's ptrdiff_t instead intptr_t as discussed in the corresponding libxml2 bug.

2) IIRC, timsort.h isn't used in libxslt, only in libxml2. This is third-party code and the compiler warning should be fixed once we upgrade to a newer version.

3) Why the `#ifdef __MINGW32__`? These fixes should improve the MSVC build as well. Also, I'd prefer plain C types instead of Windows typedefs: `char *` instead of `LPSTR`, etc.

4) I'm not sure why xsltproc.c includes winsock2.h at all. Probably not for Winsock functions but simply to get some typedefs missing on Windows. What happens if the include statement is removed completely?

How does the change to security.c fix a compiler warning?

The fallthrough warnings are enabled with newer GCC versions. They should either be disabled or fixed with the required comments.

The function attribute and set-but-unused variable warnings in python/libxslt.c also affect UNIX builds and should be fixed separately.

Created attachment 360707 [details] [review]
Updated proposed patch to fix some issues I described.

Okay.

1) I tried to fix this with the updated patch.
2) timsort.h, you are correct.  I did have to patch it in libxml2 but that's not relevant here.

3) The ifdef __MINGW32__ was there because I was only working with mingw and mingw-w64 GCC 7.2 in the MSYS2 environment and I wanted to err on the side of caution.

4) I think the winsock2 thing is that you have code that uses the timeval structure.  That is part of the Winsock (BSD sockets) specification.  Without the winsock2 header, Windows would likely default to the old winsock.h header.  I'm not entirely sure if that is desirable either because the winsock.h and winsock2.h headers are mutually exclusive.  Ideally, you would probably want to rethink what you were doing in Windows that was using gettimeofday so you could sidestep the issue entirely.  You would probably want to do something like use GetLocalTime (see: https://msdn.microsoft.com/en-us/library/windows/desktop/ms724338(v=vs.85).aspx ) but I have not done so in that patch.

5) I now recall the issue with security.h.  It had an if statement that gave a warning about guarding:

    if ((uri->scheme == NULL) ||
	(xmlStrEqual(BAD_CAST uri->scheme, BAD_CAST "file"))) {

#if defined(WIN32) && !defined(__CYGWIN__)
    if ((uri->path)&&(uri->path[0]=='/')&&
        (uri->path[1]!='\0')&&(uri->path[2]==':'))
    ret = xsltCheckWritePath(sec, ctxt, uri->path+1);
    else
#endif

	/*
	 * Check if we are allowed to write this file
	 */
	ret = xsltCheckWritePath(sec, ctxt, uri->path);
	if (ret <= 0) {
	    xmlFreeURI(uri);
	    return(ret);
	}
    } else {
[snip]
    }

and to be honest, that statement in the if defined could cause readability issues even it's technically correct.  I tried changing it to this:

    if ((uri->scheme == NULL) ||
	(xmlStrEqual(BAD_CAST uri->scheme, BAD_CAST "file"))) {
#if defined(WIN32) && !defined(__CYGWIN__)
	if ((uri->path)&&(uri->path[0]=='/')&&
        (uri->path[1]!='\0')&&(uri->path[2]==':'))
           ret = xsltCheckWritePath(sec, ctxt, uri->path+1);
	else
	   ret = xsltCheckWritePath(sec, ctxt, uri->path);
#else

	/*
	 * Check if we are allowed to write this file
	 */
	ret = xsltCheckWritePath(sec, ctxt, uri->path);
#endif
	if (ret <= 0) {
	    xmlFreeURI(uri);
	    return(ret);
	}
    } else {
[snip]
    }

I did remove that issue with the unitialized m value in python/libxslt.c since the code did match what was described in the Python API for extensions.  There was an issue with hash_code and a typecast to long that I found problematic but that's item 1.

Should be fixed with https://git.gnome.org/browse/libxslt/commit/?id=8760bb227604ed8e096fdc3cee249fb655c7cdbc