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

Add -mwin32 flag on Windows targets #19

Merged
merged 1 commit into from
Dec 30, 2014

Conversation

abonander
Copy link
Contributor

See time-rs/time#45.

I figured adding it to the defaults would be the most controllable way of implementing this fix. That way users can opt-out if it's inducing unwanted behavior. I'll reboot into Windows in a second to test this.

Edit: I can't exactly attest to the fix as Cargo silences the prints from gcc-rs, however it does build correctly even after removing the patch I made to time, so I presume it works.


if cfg!(target_os = "windows") {
flags.push("-mwin32".to_owned());
}
Copy link
Member

Choose a reason for hiding this comment

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

This may actually prefer to go in the compile_library function because otherwise this may get overridden by accident. This should also use the value of the TARGET environment variable instead of cfg! due to cross compilations that may be happening.

@abonander
Copy link
Contributor Author

What value of TARGET should I match then?

@alexcrichton
Copy link
Member

target.contains("windows") should probably be good enough for now.

@abonander
Copy link
Contributor Author

@alexcrichton Updated. r?

Couple notes:

  • Config currently doesn't need an explicit impl for Default since it can simply derive it (default for Vec<T> of any T is an empty vector).
  • All the .as_sllice() calls are redundant because of auto-deref.
  • Is calling .to_string() on a string slice better than ToOwned::to_owned()? The former has to go through the formatting framework while the latter just copies into a new allocation. Not sure how it optimizes down.

@alexcrichton
Copy link
Member

Config currently doesn't need an explicit impl for Default since it can simply derive it

I'd be down with that!

All the .as_sllice() calls are redundant because of auto-deref.

Ah, the wonders of Rust code written a few months ago :)

Is calling .to_string() on a string slice better than ToOwned::to_owned()?

For now on very large strings, it will be slower. Once rust-lang/rfcs#526 is implemented, however, this will largely no longer be true (the performance won't be the same, but the gap will be of a constant factor, not O(n))

alexcrichton added a commit that referenced this pull request Dec 30, 2014
Add `-mwin32` flag on Windows targets
@alexcrichton alexcrichton merged commit 4d09756 into rust-lang:master Dec 30, 2014
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