Skip to content

swift-reflection-test: preprocess for windows build #16914

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

Merged
merged 1 commit into from
Nov 7, 2018

Conversation

compnerd
Copy link
Member

Windows does not support fork/exec, so preprocess away the calls to
those APIs on Windows. This allows for building more of the test
binaries.

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

Windows does not support fork/exec, so preprocess away the calls to
those APIs on Windows.  This allows for building more of the test
binaries.
@compnerd
Copy link
Member Author

compnerd commented Nov 7, 2018

@swift-ci please test and merge

@jrose-apple
Copy link
Contributor

Can you leave comments for why Windows is different?

@swift-ci swift-ci merged commit 094b749 into swiftlang:master Nov 7, 2018
@compnerd compnerd deleted the fork branch November 8, 2018 02:14
@compnerd
Copy link
Member Author

compnerd commented Nov 8, 2018

@jrose-apple - I'm not sure what comments you had in mind. The current implementation is pretty heavily posix, which Win32 is not (even stuff that exists is different from standard unixy implementations, e.g. pipe). Happy to add comments about what you think would be useful though.

@@ -441,6 +453,8 @@ int reflectExistential(SwiftReflectionContextRef RC,
int doDumpHeapInstance(const char *BinaryFilename) {
PipeMemoryReader Pipe = createPipeMemoryReader();

#if defined(_WIN32)
Copy link
Contributor

Choose a reason for hiding this comment

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

Right here's where I'd want a comment, I think, even if it's just "Windows doesn't have an equivalent to fork, so we'll have to do this in-process."

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.

3 participants