Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix NullReferenceExceptions in IndentedTextWriter without writer - #39584

Merged
stephentoub merged 1 commit into
dotnet:masterfrom
hughbe:fix-nre-indentedwriter
Jul 21, 2019
Merged

stephentoub merged 1 commit into
dotnet:masterfrom
hughbe:fix-nre-indentedwriter

Conversation

@hughbe

@hughbe hughbe commented Jul 18, 2019

Copy link
Copy Markdown

Wouldn't throw NREs in the constructor but whenever any method was called on IndentedTextWriter. Fix this to validate in the constructor

@stephentoub stephentoub 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. @JeremyKuhne, are you ok with this change? Technically it's breaking (e.g. someone constructing an IndentedTextWriter with null and then never using it will now see exceptions they didn't before, and even if they do use it, the exception will now be in a different place), but I agree with @hughbe that the revised behavior is better.

@stephentoub stephentoub reopened this Jul 18, 2019

@JeremyKuhne JeremyKuhne 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.

Thanks @hughbe!

@JeremyKuhne

Copy link
Copy Markdown
Member

are you ok with this change?

Yes, much better to get the error up front.

@stephentoub

Copy link
Copy Markdown
Member

/azp run corefx-ci

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@stephentoub
stephentoub merged commit 52f0a91 into dotnet:master Jul 21, 2019
@hughbe
hughbe deleted the fix-nre-indentedwriter branch July 21, 2019 07:12
@karelz karelz added this to the 5.0 milestone Aug 3, 2019
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants