Skip to content

Use pattern-matching in MetadataWriter for readability and possibly performance.#36219

Merged
gafter merged 4 commits into
dotnet:masterfrom
gafter:master-pat1
Jul 4, 2019
Merged

Use pattern-matching in MetadataWriter for readability and possibly performance.#36219
gafter merged 4 commits into
dotnet:masterfrom
gafter:master-pat1

Conversation

@gafter

@gafter gafter commented Jun 6, 2019

Copy link
Copy Markdown
Member

No description provided.

@gafter gafter requested a review from a team as a code owner June 6, 2019 22:32
@gafter gafter self-assigned this Jun 6, 2019

IEventDefinition eventDef = definition as IEventDefinition;
if (eventDef != null)
return definition switch

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

definition [](start = 19, length = 10)

:)

case ITypeReference marshallerTypeRef:
this.SerializeTypeName(marshallerTypeRef, writer);
break;
case { }:

@CyrusNajmabadi CyrusNajmabadi Jun 6, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

not: case string s? #ByDesign

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I was trying not to modify the semantics of the code as written.


In reply to: 291407933 [](ancestors = 291407933)

@CyrusNajmabadi

Copy link
Copy Markdown
Contributor

Did you use the IDE features for this?

@gafter

gafter commented Jun 6, 2019

Copy link
Copy Markdown
Member Author

For a few of them, yes.


In reply to: 499704024 [](ancestors = 499704024)

@jcouv jcouv self-assigned this Jun 14, 2019
case ITypeReference marshallerTypeRef:
this.SerializeTypeName(marshallerTypeRef, writer);
break;
case { }:

@jcouv jcouv Jun 14, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: I think we prefer the more specific case object: over case { }: #Closed

@jcouv jcouv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 1) with a suggestion on case { }

@gafter

gafter commented Jun 28, 2019

Copy link
Copy Markdown
Member Author

@dotnet/roslyn-compiler May I please have a second review from the compiler team for this tiny bug fix?

@jcouv jcouv added this to the 16.3.P2 milestone Jun 28, 2019
@gafter gafter merged commit 1da56fd into dotnet:master Jul 4, 2019
canton7 added a commit to canton7/roslyn that referenced this pull request Jul 9, 2019
…ncat-order

* upstream/master: (1532 commits)
  Ensure we have stack spilling support for the recently-added expression node `BoundReadOnlySpanFromArray` (dotnet#37057)
  Review feedback
  Avoid generating or emitting NullablePublicOnlyAttribute when no other nullable attributes are emitted (dotnet#37019)
  Respond to more feedback
  Fixes from feedback
  Call `.WithoutNullability()` less
  Fix ngen for assets from nuget
  Make NullableWalker aware of calls to Interlocked.CompareExchange
  fix error
  Update AnonymousObjectCreation implementation to be more robust to errors by using MemberIndexOpt. Address minor PR comments.
  Add support to ngen assets included from nuget package
  Modified node removal to keep original leading trivia
  Fix Solution.WithDocumentFilePath not updating the file path of the tree
  Improve docs.
  PR Feedback cleanup.
  Use pattern-matching in MetadataWriter for readability and possibly performance. (dotnet#36219)
  Renamed helpers in SyntaxFactsService.
  More RefactoringHelpers tests (mostly for extraction).
  Add general tests for RefactoringHelpersService.
  Optimize flow analysis assembly
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants