Fix leaks in AnalyzerDriver introduced in #673 and refactor code in analyzer driver #767
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Merging this in as there are 8 broken tests (couple still failing in builds) which this change will fix. I will address CR feedback subsequently. |
mavasani
added a commit
that referenced
this pull request
Feb 22, 2015
My prior change #673 made to address issues with analyzer exception diagnostics introduced a few leaks in product and test code. The primary reason was that I attempted to use static events to track state as the existing AnalyzerDriverHelper type which did the core analyzer execution was a static type with all static methods. Additionally, I added a static event to the new host diagnostic update source added for reporting analyzer specific diagnostics. These were holding onto projects/workspaces in test runs causing leaks. I have reverted the approach of static events and instead refactored the code in AnalyzerDriver project to simplify the whole design: 1. Renamed AnalyzerDriverHelper to AnalyzerExecutor and made it a non-static type, which has instance fields for all the configuration parameters for analyzer callbacks. 2. Move all the core analyzer callbacks (actions/Initialize method/supported diagnostics) into AnalyzerExecutor. Command line compiler just creates a single instance of the executor, while IDE driver creates instances per analyzer. 3. Both these changes simplified the API a lot, and I just had to add an additional field "Action addExceptionDiagnostic" to AnalyzerExecutor to configure how to handle exception diagnostics. 4. MEF import AbstractHostDiagnosticUpdateSource in DiagnosticAnalyzerService and thread it down to IDE analyzer driver. 5. Changes 3 and 4 above meant that in the IDE driver, delegate "addExceptionDiagnostic" just asks the HostDiagnosticUpdateSource to report the exception diagnostic produced by the AnalyzerExecutor. This change also re-enables the tests skipped by #761 and fixes #759.
|
👍 by the way, is your previous refactoring could cause what to run for compilation end analyzer to be changed? I have this RPS rebuild test regression and I am fixing (or trying to fix it) at engine but still plan to figure out what caused the behavior change. basically, I think we are running code only for compilation end actions even when there is none. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
My prior change #673 made to address issues with analyzer exception diagnostics introduced a few leaks in product and test code. The primary reason was that I attempted to use static events to track state as the existing AnalyzerDriverHelper type which did the core analyzer execution was a static type with all static methods. Additionally, I added a static event to the new host diagnostic update source added for reporting analyzer specific diagnostics. These were holding onto projects/workspaces in test runs causing leaks.
I have reverted the approach of static events and instead refactored the code in AnalyzerDriver project to simplify the whole design:
This change also re-enables the tests skipped by #761 and fixes #759.
Lessons learnt: