Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zlibWrapper/*.c files cannot be compiled with VS #1800

Closed
bluenlive opened this issue Sep 18, 2019 · 9 comments · Fixed by #1847
Closed

zlibWrapper/*.c files cannot be compiled with VS #1800

bluenlive opened this issue Sep 18, 2019 · 9 comments · Fixed by #1847
Assignees
Labels

Comments

@bluenlive
Copy link

bluenlive commented Sep 18, 2019

Thank you for your great work!

Visual studio (any version) cannot compile *.c files in zlibWrapper folder.

Following line(s)(25 times) requires -Wstrict-aliasing=1 option.
state = (gz_statep)file;

But, VS doesn't support strict aliasing.
So, I think it's better to change those lines to:
*(void**)(&state) = (void*)file;
or
state.file = file;

And, VS doesn't define ssize_t type, but it is used in gzread.c
So, something like this is needed in gzread.c
#if defined(_MSC_VER) && !defined(ssize_t)
#include <BaseTsd.h>
typedef SSIZE_T ssize_t;
#endif

What do you think about these issue?

@Cyan4973
Copy link
Contributor

Cyan4973 commented Sep 25, 2019

We are missing a build test of the zlibWrapper using Visual Studio compiler.
This would help trap, and then fix, such issue.

I suspect we could add it somewhere here :
https://github.com/facebook/zstd/blob/dev/appveyor.yml#L152

@Cyan4973
Copy link
Contributor

Also, looking at the build script, it seems the source code is created to be compatible with gnu99 :
https://github.com/facebook/zstd/blob/dev/zlibWrapper/Makefile#L23

This is not strict ansi c90, therefore there might be some issues with other compilers, including Visual Studio.

@Cyan4973
Copy link
Contributor

Cyan4973 commented Sep 25, 2019

In branch zlibwrap, there is an updated version of the zlib wrapper, featuring must stricter conformity with C90.

It should generate less warnings for Visual Studio.

Limitations :
There is still no automated test to compile the zlib wrapper with Visual Studio on AppveyorCI.
It also doesn't address the ssize_t issue.

@Cyan4973
Copy link
Contributor

Cyan4973 commented Oct 4, 2019

While I could ensure C90 conformity in #1815 ,
on Window, I'm mostly limited to mingw compilation tests for the time being,
as the Visual environment is quite different.

@Cyan4973
Copy link
Contributor

Cyan4973 commented Oct 17, 2019

In latest dev branch update, zlibWrapper is now ISO-C90 compatible,
which should make it more compatible with Visual.

I've tested that the new source code compiles without warning on Visual Studio 2017. zlibWrapper is also added to our CI test suite.

But I can't reproduce more specific issues such as linking a specific version of zlib within a Visual project using a specific version of Visual compiler.

@bluenlive, if you have time, could you please have a look at latest zstd update in dev branch, and see if it solves or improves the situation for you ?
And if not, what needs to be done to fix it ?

@brplummer
Copy link

@Cyan4973, your statement above:

In latest dev branch update, zlibWrapper is not ISO-C90 compatible

should be:

In latest dev branch update, zlibWrapper is NOW ISO-C90 compatible

Correct?

@Cyan4973
Copy link
Contributor

correct, fixed

@bluenlive
Copy link
Author

@Cyan4973 Sorry for my late answer.

In VS2019, one more fix is needed.
ssize_t is not defined in VS. (As you know, it's not in standard C)
So, gzread.c cannot be compiled with VS.

I inserted following lines, and compilation was done.

#if defined(_MSC_VER) && !defined(ssize_t)
#include <BaseTsd.h>
typedef SSIZE_T ssize_t;
#endif

Cyan4973 added a commit that referenced this issue Oct 24, 2019
As per #1800 (comment),
Visual does not support `ssize_t` type,
which is an issue for `gzread.c`.

Added a work around, suggested by @bluenlive

Note : I have not been able to confirm the problem,
so this is a blind fix.
This seems safe outside of Visual, since it is gated by _MSC_VER macro.
@bluenlive
Copy link
Author

@Cyan4973 Thank you so much.
Now it is compiled with VS2019! (And I'm sure with previous versions!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants