Skip to content

[stable/20221013 Cherry Pick] [clang][deps] Remove unintentional move #5590

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

Closed

Conversation

artemcm
Copy link

@artemcm artemcm commented Nov 10, 2022

Originally fixed by @jansvoboda11 in https://reviews.llvm.org/D136124

This is a fix related to D135414. The original intention was to keep BaseFS as a member of the worker and conditionally overlay it with local in-memory FS. The move of ref-counted BaseFS was not intended, and it's a bug.

Disabling parallelism in the "by-module-name" test reliably reproduces this, and the test itself doesn't need parallelism. (I think -j 4 was cargo culted from another test.) Reusing that test to check for correct behavior...

Reviewed By: DavidSpickett

Differential Revision: https://reviews.llvm.org/D136124

This is a fix related to D135414. The original intention was to keep `BaseFS` as a member of the worker and conditionally overlay it with local in-memory FS. The `move` of ref-counted `BaseFS` was not intended, and it's a bug.

Disabling parallelism in the "by-module-name" test reliably reproduces this, and the test itself doesn't *need* parallelism. (I think `-j 4` was cargo culted from another test.) Reusing that test to check for correct behavior...

Reviewed By: DavidSpickett

Differential Revision: https://reviews.llvm.org/D136124
@artemcm artemcm requested a review from jansvoboda11 November 10, 2022 00:02
@artemcm
Copy link
Author

artemcm commented Nov 10, 2022

rdar://102152282

@artemcm
Copy link
Author

artemcm commented Nov 10, 2022

@swift-ci test

@jansvoboda11
Copy link

Is this necessary? I already have a PR up: #5568.

@artemcm artemcm closed this Nov 10, 2022
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