Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix ssize_t on Windows #96

Closed
wants to merge 1 commit into from
Closed

Conversation

ales-tsurko
Copy link

Rust's isize converted into ssize_t, but it's not defined on Windows. That leads to compilation errors. There's SSIZE_T though.

Rust `isize` converted into `ssize_t`, but it's not defined on Windows. That leads to compilation errors. There's `SSIZE_T` though.
Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Were you able to confirm that this fixes your problem? It looks like it doesn't to me, since this header does not get included by the generated cc file.

@myronahn
Copy link
Contributor

I have a similar fix in my pull request. It does seem to fix the problem although it could be that the header just happened to be included in all the right places.

15b5b10

@dtolnay
Copy link
Owner

dtolnay commented Apr 10, 2020

Ah I guess this relies on at least one of the include!'d files to contain #include "rust/cxx.h", which wouldn't always be the case.

Is there a meaningful difference between using _WIN32 vs _MSC_VER?

Alternatively we could do this based on env::var_os("CARGO_CFG_WINDOWS").is_some() for Cargo builds since Cargo guarantees that is passed for all Windows targets, and use something like a --windows flag for the cxxbridge cli command for the case of non-Cargo builds.

@dtolnay
Copy link
Owner

dtolnay commented Apr 10, 2020

I think I prefer @myronahn's use of a type alias instead of a macro for this. But I would prefer not to have our header define ssize_t in the root namespace. Could we define it as rust::isize instead, using ssize_t as the type in the non-Windows case?

@dtolnay
Copy link
Owner

dtolnay commented Apr 10, 2020

I've published cxx 0.2.7 containing the fix from #97.

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

Successfully merging this pull request may close these issues.

3 participants