Skip to content
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

Make SemanticModel's Analyze*Flow methods public. Remove corresponding extension methods. #187

Closed
JoshVarty opened this issue Jan 31, 2015 · 5 comments
Labels
Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request Resolution-By Design The behavior reported in the issue matches the current design Verified

Comments

@JoshVarty
Copy link
Contributor

Both AnalyzeControlFlow and AnalyzeDataFlow are marked internal within SemanticModel.

They're exposed via extension methods in ModelExtensions.AnalyzeControlFlow and ModelExtension.AnalyzeDataFlow

I don't think accessing these methods through extension methods provides any value. I propose the extension methods be removed (for the above methods, and their other overload) and we simply mark the corresponding methods within SemanticModel as public.

If the team agrees, I can go ahead and submit a PR for this.

@JoshVarty JoshVarty changed the title Make SemanticModel 's Analyze*Flow methods public. Remove extension methods. Make SemanticModel's Analyze*Flow methods public. Remove extension methods. Jan 31, 2015
@JoshVarty JoshVarty changed the title Make SemanticModel's Analyze*Flow methods public. Remove extension methods. Make SemanticModel's Analyze*Flow methods public. Remove corresponding extension methods. Jan 31, 2015
@jmarolf jmarolf added Concept-API This issue involves adding, removing, clarification, or modification of an API. Area-Compilers Enhancement help wanted The issue is "up for grabs" - add a comment if you are interested in working on it labels Jan 31, 2015
@jmarolf
Copy link
Contributor

jmarolf commented Jan 31, 2015

This looks like a mistake from when we unified the interface ISemanticModel to the abstract class SemanticModel. Go ahead and submit a PR.

@gafter
Copy link
Member

gafter commented Jan 31, 2015

If I recall, the language-specific extension methods are type safe (e.g. Taking language-specific StatementSyntax)

@JoshVarty
Copy link
Contributor Author

There are language-specific extension methods and they should be left alone.

But I don't think that the language-agnostic extension methods serve a purpose that wouldn't be achieved by making the following methods public instead of internal.

SemanticModel.AnalyzeControlFlow(SyntaxNode)
SemanticModel.AnalyzeControlFlow(SyntaxNode, SyntaxNode)
SemanticModel.AnalyzeDataFlow(SyntaxNode)
SemanticModel.AnalyzeDataFlow(SyntaxNode, SyntaxNode)

@DustinCampbell
Copy link
Member

That seems fair to me.

@gafter gafter self-assigned this Feb 6, 2015
@JoshVarty
Copy link
Contributor Author

As discussed in #190 this is by design.

@gafter gafter added Resolution-By Design The behavior reported in the issue matches the current design 4 - In Review A fix for the issue is submitted for review. and removed help wanted The issue is "up for grabs" - add a comment if you are interested in working on it 4 - In Review A fix for the issue is submitted for review. labels Feb 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request Resolution-By Design The behavior reported in the issue matches the current design Verified
Projects
None yet
Development

No branches or pull requests

5 participants