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
