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

Remove MSVC's wmemchr declaration #1698

Merged
merged 1 commit into from
Mar 18, 2020

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Mar 18, 2020

No description provided.

@rust-highfive
Copy link

r? @gnzlbg

(rust_highfive has picked a reviewer for you, use r? to override)

@tesuji
Copy link
Contributor Author

tesuji commented Mar 18, 2020

r? @JohnTitor

@tesuji
Copy link
Contributor Author

tesuji commented Mar 18, 2020

Style (lint) check failed

+ ./style src
src/windows/mod.rs:383 - use cfg_if! and submodules instead of #[cfg]

What should I do? There are no way to link to wmemchr on MSVC.

@igrep
Copy link
Contributor

igrep commented Mar 18, 2020

I guess you should move the function you modified to https://github.com/rust-lang/libc/blob/master/src/windows/gnu/mod.rs
src/windows/mod.rs should contain only functions common in the Windows targets.

@tesuji
Copy link
Contributor Author

tesuji commented Mar 18, 2020

Thanks. Guess it should be fine now.

Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

Hm, it'd be great if CI could detect this absence. cc @retep998 are we correct?

src/windows/gnu/mod.rs Outdated Show resolved Hide resolved
@retep998
Copy link
Member

I'm surprised CI hadn't detected this. The change looks correct to me.

@JohnTitor
Copy link
Member

Thanks, @retep998!

I'm surprised CI hadn't detected this.

Yeah, I'll take a look later.

@lzutao r=me once code is formatted, line length limit here is tight a bit.
Also, thanks @igrep for the comment.

@tesuji
Copy link
Contributor Author

tesuji commented Mar 18, 2020

CI could detect this absence

I think that the extern block relates to linkage, which happen during linking stage and after codegen.
IMO we cannot test this case unless we have tests for every libc function.

@JohnTitor
Copy link
Member

IMO we cannot test this case unless we have tests for every libc function.

Well CI should do that with ctest crate iirc?

Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

Anyway I'm going to merge this, thanks all!

@JohnTitor JohnTitor merged commit 276eaa2 into rust-lang:master Mar 18, 2020
@tesuji tesuji deleted the non-msvc-wmemchr branch March 18, 2020 10:59
@tesuji
Copy link
Contributor Author

tesuji commented Mar 18, 2020

Just curious: So we don't use bors anymore.

@JohnTitor
Copy link
Member

Just curious: So we don't use bors anymore.

Yeah, Alex wants to abandon the use of bors here, I think we'll do so if there are no issues for a while.

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.

6 participants