Skip to content

[Diagnostics] Rely on clang headers for .dia block and record IDs #31849

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
May 18, 2020

Conversation

owenv
Copy link
Contributor

@owenv owenv commented May 17, 2020

This makes it clearer Swift doesn't control the format, and prevents drift between the two sets of constants, which has been a source of bugs in the past.

@@ -334,7 +302,7 @@ void SerializedDiagnosticConsumer::emitMetaBlock() {
Stream.EnterSubblock(BLOCK_META, 3);
Record.clear();
Record.push_back(RECORD_VERSION);
Record.push_back(Version);
Record.push_back(VersionNumber);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: This is 2 now, looking at git history, swift has incorrectly reported 1 since 2014. AFAICT, this is safe to change because no clients actually check it. If that's not the case, I can revert this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. Let's qualify this clang::serialized_diags::VersionNumber

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good to me, done

@owenv
Copy link
Contributor Author

owenv commented May 17, 2020

@swift-ci smoke test

@owenv owenv requested a review from CodaFi May 17, 2020 18:42
Copy link
Contributor

@CodaFi CodaFi left a comment

Choose a reason for hiding this comment

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

Modulo the explicit qualification, this is good to merge. Thanks!

@owenv
Copy link
Contributor Author

owenv commented May 18, 2020

@swift-ci smoke test

@owenv
Copy link
Contributor Author

owenv commented May 18, 2020

Windows run passed but wasn't reported: https://ci-external.swift.org/view/Pull%20Request/job/swift-PR-windows/3138/

@owenv owenv merged commit f3cc6a5 into swiftlang:master May 18, 2020
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