-
Notifications
You must be signed in to change notification settings - Fork 231
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
Improve performance for SymbolReferenceAnalyzer #5548
Improve performance for SymbolReferenceAnalyzer #5548
Conversation
[DataTestMethod] | ||
[DataRow(true)] | ||
[DataRow(false)] | ||
public void GetSetKeyword_ReturnsNull_VB(bool isTestProject) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"set" keyword was supported only for adding support for "value". This complicated the logic and it was not consistent. The "set" declaration was added only in the presence of "value".
I prefer to remove this part of the functionality and keep the logic simple.
@@ -101,7 +101,7 @@ public class SymbolReferenceAnalyzerTest | |||
[DataRow(ProjectType.Product)] | |||
[DataRow(ProjectType.Test)] | |||
public void Verify_NamedType_CS(ProjectType projectType) => | |||
Verify("NamedType.cs", projectType, 4, 3, 7, 7); // 'var' and type name on the same line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"var" here was redirecting to the underlying type, which was nice. I've dropped the support, for now, to keep the logic simple but I might change my mind until if finish with this rework.
30179bf
to
f2b464a
Compare
1d93ac6
to
0578574
Compare
analyzers/src/SonarAnalyzer.Common/Rules/Utilities/SymbolReferenceAnalyzerBase.cs
Show resolved
Hide resolved
0578574
to
da188ec
Compare
analyzers/src/SonarAnalyzer.Common/Rules/Utilities/SymbolReferenceAnalyzerBase.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/Utilities/SymbolReferenceAnalyzerBase.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One additional performance improvement suggested.
I've dismissed myself from the reviewers, I'm just a kibitzer here. My threads are resolved now 🎉 |
03fd471
to
bd51ce7
Compare
analyzers/src/SonarAnalyzer.Common/Rules/Utilities/SymbolReferenceAnalyzerBase.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/Utilities/SymbolReferenceAnalyzerBase.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left the last two comments before approval.
Kudos, SonarCloud Quality Gate passed! |
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Rewrite the analyzer to work as much as possible at the syntax level by retrieving symbols only when strictly necessary.
Tested this rule on:
Part of #4220