Skip to content

[Diagnostics] Refactor DiagnosticConsumer interface to remove unnecessary params #27868

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 30, 2019
Merged

[Diagnostics] Refactor DiagnosticConsumer interface to remove unnecessary params #27868

merged 1 commit into from
Oct 30, 2019

Conversation

owenv
Copy link
Contributor

@owenv owenv commented Oct 24, 2019

DiagnosticInfo now holds all the information needed to consume a diagnostic, so remove unneeded parameters from handleDiagnostic.

I've been meaning to clean this up for a while, it should make future changes a lot easier. I made sure everything builds and the unit tests pass, but there's always a chance I missed a consumer somewhere in one of the tools.

@owenv
Copy link
Contributor Author

owenv commented Oct 29, 2019

cc @jrose-apple , do you mind taking a quick look at these changes? It looks like you're the original author of the unit tests.

jrose-apple
jrose-apple approved these changes Oct 29, 2019
View reviewed changes
Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

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

Seems pretty mechanical of a change, and definitely simplifies things.

@owenv
[Diagnostics] Refactor DiagnosticConsumer interface
8a67117 
DiagnosticInfo now holds all the information needed to consume
a diagnostic, so remove unneeded parameters from handleDiagnostic.
@owenv owenv force-pushed the diag-info-refactor branch from 03fbd0c to 8a67117 Compare October 29, 2019 20:52
@jrose-apple
Copy link
Contributor

@swift-ci Please test

@jrose-apple jrose-apple self-assigned this Oct 29, 2019
@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 03fbd0c5466aa46d7577d75be0316a7192b0a031

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 03fbd0c5466aa46d7577d75be0316a7192b0a031

@owenv
Copy link
Contributor Author

owenv commented Oct 29, 2019

Looks like I need to update swift-lldb as well, I'll put together a PR

@owenv owenv mentioned this pull request Oct 30, 2019
Merged
@owenv
Copy link
Contributor Author

owenv commented Oct 30, 2019

@jrose-apple assuming I understand the new monorepo branch setup correctly, this should build now with swiftlang/llvm-project#58

@jrose-apple
Copy link
Contributor

swiftlang/llvm-project#58
@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 8a67117

@jrose-apple
Copy link
Contributor

swiftlang/llvm-project#58
@swift-ci Please test macOS

@jrose-apple jrose-apple merged commit 5c2185f into swiftlang:master Oct 30, 2019
@compnerd
Copy link
Member

This may have caused a regression on the Windows build: https://ci-external.swift.org/job/oss-swift-windows-x86_64/1790/console

@drodriguez drodriguez mentioned this pull request Oct 31, 2019
Merged
@owenv
Copy link
Contributor Author

owenv commented Oct 31, 2019

@compnerd yeah, this was a silly mistake on my part. I think @drodriguez's fix should work

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@jrose-apple jrose-apple jrose-apple approved these changes

Assignees

@jrose-apple jrose-apple

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@owenv @jrose-apple @swift-ci @compnerd