Skip to content

Avoid trying to simplify descendant identifier names#41072

Merged
sharwell merged 1 commit intodotnet:masterfrom
sharwell:filter-identifier-name
Jan 21, 2020
Merged

Avoid trying to simplify descendant identifier names#41072
sharwell merged 1 commit intodotnet:masterfrom
sharwell:filter-identifier-name

Conversation

@sharwell
Copy link
Copy Markdown
Contributor

@sharwell sharwell commented Jan 18, 2020 •
edited
Loading

These identifiers are already simplified as part of nodes further up the tree.

Extracted from #40746.

Baseline numbers (49a468c):

Found 24193 diagnostics in 102883ms (150147571248 bytes allocated)
Execution times (ms):
CSharpSimplifyTypeNamesDiagnosticAnalyzer:      2190940
  SimpleMemberAccessExpression: 1170342ms to try simplifying 1383556 instances
  IdentifierName: 605398ms to try simplifying 4466446 instances
  GenericName: 146827ms to try simplifying 92323 instances
  QualifiedName: 68286ms to try simplifying 158081 instances
  QualifiedCref: 434ms to try simplifying 1245 instances
  AliasQualifiedName: 10ms to try simplifying 30 instances
VisualBasicSimplifyTypeNamesDiagnosticAnalyzer: 1764312
  SimpleMemberAccessExpression: 1236160ms to try simplifying 632416 instances
  IdentifierName: 353337ms to try simplifying 1860769 instances
  GenericName: 64360ms to try simplifying 32776 instances
  QualifiedName: 55078ms to try simplifying 62234 instances

Updated numbers (49a468c + this pull request):

Found 24193 diagnostics in 97737ms (148431953600 bytes allocated)
Execution times (ms):
CSharpSimplifyTypeNamesDiagnosticAnalyzer:      1552142
  SimpleMemberAccessExpression: 917073ms to try simplifying 1383559 instances
  IdentifierName: 321246ms to try simplifying 2951512 instances
  GenericName: 127254ms to try simplifying 92323 instances
  QualifiedName: 60160ms to try simplifying 158084 instances
  QualifiedCref: 238ms to try simplifying 1245 instances
  AliasQualifiedName: 10ms to try simplifying 30 instances
VisualBasicSimplifyTypeNamesDiagnosticAnalyzer: 1528633
  SimpleMemberAccessExpression: 1098896ms to try simplifying 632418 instances
  IdentifierName: 272517ms to try simplifying 1197747 instances
  GenericName: 51408ms to try simplifying 32776 instances
  QualifiedName: 48384ms to try simplifying 62234 instances

@sharwell sharwell requested a review from a team as a code owner January 18, 2020 20:44
@sharwell sharwell force-pushed the filter-identifier-name branch from 93e43dc to c3334bc Compare January 18, 2020 20:45
CyrusNajmabadi
CyrusNajmabadi reviewed Jan 18, 2020
View reviewed changes
Comment thread src/Features/CSharp/Portable/Diagnostics/Analyzers/TypeSyntaxSimplifierWalker.cs Outdated
@sharwell sharwell force-pushed the filter-identifier-name branch from c3334bc to da6f01f Compare January 18, 2020 23:12
CyrusNajmabadi
CyrusNajmabadi reviewed Jan 18, 2020
View reviewed changes
Comment thread src/Features/CSharp/Portable/Diagnostics/Analyzers/TypeSyntaxSimplifierWalker.cs Outdated
@CyrusNajmabadi @sharwell
Avoid trying to simplify descendant identifier names
423dd1b 
These identifiers are already simplified as part of nodes further up the
tree.
@sharwell sharwell force-pushed the filter-identifier-name branch from da6f01f to 423dd1b Compare January 18, 2020 23:18
@sharwell sharwell mentioned this pull request Jan 19, 2020
Merged
mavasani
mavasani reviewed Jan 21, 2020
View reviewed changes
Comment thread src/Features/CSharp/Portable/Diagnostics/Analyzers/TypeSyntaxSimplifierWalker.cs
//
// In other cases, don't bother looking at the right side of A.B or A::B. We will process those in
// one of our other top level Visit methods (like VisitQualifiedName).
var canTrySimplify = node.Identifier.ValueText!.EndsWith("Attribute", StringComparison.Ordinal)
Copy link
Copy Markdown
Contributor

@mavasani mavasani Jan 21, 2020

Choose a reason for hiding this comment

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

Unrelated to this PR, but when can Identifier.ValueText be null?

Copy link
Copy Markdown
Contributor Author

@sharwell sharwell Jan 21, 2020

Choose a reason for hiding this comment

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

default(SyntaxToken)

Though I'm planning to propose changing this behavior.

@sharwell sharwell merged commit a1fe543 into dotnet:master Jan 21, 2020
@sharwell sharwell deleted the filter-identifier-name branch January 21, 2020 17:25
@sharwell sharwell added this to the 16.5.P3 milestone Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@mavasani mavasani mavasani approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-IDE

Projects

None yet

Milestone

16.5.P3

Development

Successfully merging this pull request may close these issues.

3 participants

@sharwell @CyrusNajmabadi @mavasani