Hi All, I've addressed some(all?) of the comments above and rebased on 5.2.1. The relevant changes are available https://github.com/mindw/xz in the cmake branch.
Comments are welcome. -gabi -gabi On Tue, Feb 3, 2015 at 8:44 PM, Lasse Collin <lasse.col...@tukaani.org> wrote: > On 2015-01-30 Gabi Davar wrote: >> There many warnings detected by VS which I'm not sure how to fix. > > Maybe I can test with VS Community myself some day. > >> > - Are the casts added in tuklib_integer.h there to silence >> > warnings? If so, I'll add the casts. > >> [GabiD] - They silence a few warnings. > > OK, I will change it. > >> > - I don't want "restrict" keywords in the API headers. The headers >> > need to be C89 and C++98 compatible. There is nothing wrong >> > having the restricts only in the .c files. Maybe they were added >> > to silence warnings? The restricts aren't that important so >> > maybe they could even be removed, but I think the best is to >> > leave those as is unless MSVC cannot compile it at all now >> > (maybe add a flag to MSVC to omit that warning). > >> [GabiD] - They generate warnings. It's best to keep the definition and >> deceleration of a function identical. I'll revert this is an >> interface change and there are other places where it's not fixed. > > The only sane way to keep them identical would be removing the > "restrict" keywords from the .c files. It wouldn't be a big loss but it > would be a bit silly. > >> On a side note, It's unclear to me why the interfaces are to be in C89 >> since the implementation requires it (mixing C runtimes in windows >> will caused failures). > > liblzma is implemented in C99 but keeping the API headers compatible > with C89 and C++98 allows using the library from those languages too. > > I might be wrong, but doesn't it work to mix runtimes if one doesn't > expose runtime-specific stuff via the DLL API? For example, liblzma API > doesn't have any C-library-dependent declarations and it doesn't, for > example, expose file descriptor numbers or throw exceptions. I have > understood that this way it's fine to use MinGW-w64-built liblzma.dll > with other toolchains. Most of time time it doesn't work with static > libraries though, which is why people have been wishing that liblzma > could be built with Visual Studio so that they could create a static > executable that includes liblzma. For non-static uses the existing > binaries should be OK. > >> > - _snprintf isn't a safe replacement for C99 snprintf. Maybe it is >> > acceptable here since the buffers used in xz should be big >> > enough, but I have still liked using snprintf. (It's good to >> > note that snprintf is only needed for the command line tools, >> > not liblzma.) > >> [GabiD] If it's that important I'll add an implementation for pre VS >> 14. > > Well, I'm not sure what is reasonable. Note that xz uses vsnprintf too > which is equally non-standard in VS 2013, but since it exists in VS > 2013, one doesn't get an error when compiling. The non-standard > implementations don't always null-terminate the string and the return > value is non-standard in some cases. I think it shouldn't be a problem > in the current xz code, yet it leaves a slightly uncomfortable feeling. > > Hopefully there aren't other traps like this in VS 2013. On the other > hand it's good to keep in mind that the xz command line tools were > originally written with POSIX-like systems in mind. Getting liblzma to > build should be much simpler (and safer) since it doesn't need much > from the C library. liblzma is also more important than the command > line tools since the command line tools already are available for > Windows, but making them build with VS 2013 is still good, I guess. > >> > - I don't want to commit a second copy of getopt.h. Perhaps just >> > make windows/msvc2013/getopt.h #include "../../lib/getopt.in.h"? > >> [GabiD] - Since they are identical, How about renaming getopt.in.h to >> getopt.h? > > The idea of .in.h is that if configure detects that the system getopt.h > isn't good enough, a replacement getopt.h will be created. The "lib" > directory, which might contain getopt.h, is always in the compiler > include path. My understanding is that this is a common way to handle > replacement headers. > > As long as there is just a single replacement header, other tricks > could work. Since there might be other replacement headers in the > future, it's no use to change this. It's simplest to add a one-line > wrapper header for VS 2013. > > -- > Lasse Collin | IRC: Larhzu @ IRCnet & Freenode >
