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

Use a typedef instead of a #define for ssize_t with MSVC #32

Merged
merged 2 commits into from
Apr 9, 2015

Conversation

tkelman
Copy link
Contributor

@tkelman tkelman commented Apr 5, 2015

I was seeing the define for ssize_t cause some collisions and compilation errors in Julia (with MSVC) due to libuv also using this type. I think a typedef would be preferable? Could potentially use intptr_t for this instead, but I'm not sure exactly which versions of MSVC would support that.

@stevengj
Copy link
Member

stevengj commented Apr 6, 2015

A typedef also potentially collides.

Maybe wrap the #define in an #ifndef?

@tkelman
Copy link
Contributor Author

tkelman commented Apr 6, 2015

Will try it, but I don't think that'll work since the libuv headers aren't checking ssize_t, they're checking _SSIZE_T_ which they set themselves.

@tkelman
Copy link
Contributor Author

tkelman commented Apr 6, 2015

The full failure, for reference, is

 /d/code/msys64/home/Tony/julia/deps/libuv/compile cl -nologo -MD -Z7 -TP "-DJL_
SYSTEM_IMAGE_PATH=\"../lib/julia/sys.ji\"" -DJULIA_TARGET_ARCH=x86-64 -D_WIN32_W
INNT=0x0502 -I../support   -I/d/code/msys64/home/Tony/julia/deps/libuv/include -
I/d/code/msys64/home/Tony/julia/usr/include  -DLIBRARY_EXPORTS -DUTF8PROC_EXPORT
S -DNDEBUG -c julia_extensions.c -o /d/code/msys64/home/Tony/julia/src/flisp/jul
ia_extensions.o
julia_extensions.c
d:\code\msys64\home\tony\julia\deps\libuv\include\uv-win.h(27) : error C2628: 'i
ntptr_t' followed by '__int64' is illegal (did you forget a ';'?)
make[2]: *** [/d/code/msys64/home/Tony/julia/src/flisp/julia_extensions.o] Error
 2
make[2]: Leaving directory `/d/code/msys64/home/Tony/julia/src/flisp'
make[1]: *** [flisp/libflisp.a] Error 2
make[1]: Leaving directory `/d/code/msys64/home/Tony/julia/src'
make: *** [julia-src-release] Error 2

We could either use the same _SSIZE_T_ defines as libuv, or #undef ssize_t in Julia's code, but both seem a little fragile to me.

@stevengj
Copy link
Member

stevengj commented Apr 6, 2015

If we typedef our own ssize_t wouldn't that also conflict with another header that has the same typedef?

This is a general problem with defining our own symbols with standard names — other libraries are bound to use the same names.

@stevengj
Copy link
Member

stevengj commented Apr 6, 2015

I'm not sure I understand the problem. If libuv is #defining only _SSIZE_T_, why would that conflict with ssize_t?

@tkelman
Copy link
Contributor Author

tkelman commented Apr 6, 2015

If we typedef our own ssize_t wouldn't that also conflict with another header that has the same typedef?

I would think it might, but it apparently wasn't an issue at least in this limited case of utf8proc and libuv. Maybe if the typedefs are consistent enough, the compiler doesn't complain?

If libuv is #defining only _SSIZE_T_, why would that conflict with ssize_t?

See https://github.com/JuliaLang/libuv/blob/abcbb0c22faa527c427e1ea6b55f073836b26f69/include/uv-win.h#L26-L30 for precisely what libuv is doing. It's using _SSIZE_T_ as an include guard around a typedef for ssize_t. That typedef is a compilation error if included after utf8proc.h has #defined ssize_t.

@tkelman
Copy link
Contributor Author

tkelman commented Apr 6, 2015

As an alternative, we could pick a less-likely-to-collide name, something like utf8proc_ssize_t and use that everywhere.

@tkelman
Copy link
Contributor Author

tkelman commented Apr 6, 2015

That avoids the MSVC issue, and Travis and Appveyor look happy with it.

@stevengj
Copy link
Member

stevengj commented Apr 6, 2015

Picking a new name seems like the best practice. But we should also probably define utf8proc_uint8_t etcetera to be consistent?

@tkelman
Copy link
Contributor Author

tkelman commented Apr 6, 2015

Perhaps. I was wanting to avoid extra work to fix problems that I haven't had, but I could go even more nuts with grep and sed if needed. MSVC has been gradually improving in its C99 compliance recently, so many of these typedefs aren't needed with modern versions, but laziness probably isn't a good enough reason to not at least leave in the possibility of support for old versions. The Python 2 community will be continuing to use MSVC 2008 for several more years at the very least.

@tkelman
Copy link
Contributor Author

tkelman commented Apr 7, 2015

Done. This latest commit seems like overkill to me, take it or leave it.

Note that I didn't touch the contents of bench, which AFAICT won't build right now with a non-C99 compiler anyway. I have little interest in dealing with that at the moment.

@tkelman
Copy link
Contributor Author

tkelman commented Apr 9, 2015

Okay to merge this, or is that second commit too ugly?

@stevengj
Copy link
Member

stevengj commented Apr 9, 2015

Should be okay to merge.

stevengj added a commit that referenced this pull request Apr 9, 2015
Use a typedef instead of a #define for ssize_t with MSVC
@stevengj stevengj merged commit 7c14ef5 into master Apr 9, 2015
@tkelman
Copy link
Contributor Author

tkelman commented Apr 9, 2015

Thanks!

@tkelman tkelman deleted the tk/ssize_t_typedef branch April 9, 2015 23:26
@tkelman
Copy link
Contributor Author

tkelman commented Apr 10, 2015

Presumably we can merge master into the release-1.2 branch at some point.

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

Successfully merging this pull request may close these issues.

2 participants