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

Move throw calls inside ThrowHelper.Get* methods #59378

Closed
eiriktsarpalis opened this issue Sep 20, 2021 · 5 comments · Fixed by #61746
Closed

Move throw calls inside ThrowHelper.Get* methods #59378

eiriktsarpalis opened this issue Sep 20, 2021 · 5 comments · Fixed by #61746
Assignees
Labels
Milestone

Comments

@eiriktsarpalis
Copy link
Member

I believe that's indeed the point. We may need a pass at the reader/writer ThrowHelper implementation to throw inline rather than return exceptions, assuming there have been no JIT changes that render the optimization redundant.

Originally posted by @layomia in #55564 (comment)

@eiriktsarpalis eiriktsarpalis self-assigned this Sep 20, 2021
@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Sep 20, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Sep 20, 2021
@ghost
Copy link

ghost commented Sep 20, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

I believe that's indeed the point. We may need a pass at the reader/writer ThrowHelper implementation to throw inline rather than return exceptions, assuming there have been no JIT changes that render the optimization redundant.

Originally posted by @layomia in #55564 (comment)

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Text.Json, untriaged

Milestone: 7.0.0

@GrabYourPitchforks
Copy link
Member

For reference, the typical convention is to have Get* methods return an Exception instance, with Throw* methods throwing the Exception instance. See for example:

[DoesNotReturn]
internal static void ThrowArgumentException(ExceptionResource resource)
{
throw GetArgumentException(resource);
}

@eiriktsarpalis
Copy link
Member Author

eiriktsarpalis commented Sep 21, 2021

What are the use cases for exposing Get* methods? All cases I'm aware of involve throwing the instance straight away.

@GrabYourPitchforks
Copy link
Member

Maybe for wrapping exceptions? I agree it's atypical.

@eiriktsarpalis eiriktsarpalis changed the title Move throw calls inside ThrowHelper.Get* methoda Move throw calls inside ThrowHelper.Get* methods Sep 21, 2021
@eiriktsarpalis eiriktsarpalis added enhancement Product code improvement that does NOT require public API changes/additions and removed untriaged New issue has not been triaged by the area owner enhancement Product code improvement that does NOT require public API changes/additions labels Sep 23, 2021
@eiriktsarpalis eiriktsarpalis added the tenet-performance Performance related issue label Oct 15, 2021
@eiriktsarpalis
Copy link
Member Author

Maybe for wrapping exceptions? I agree it's atypical.

One use case seems to be calling throw helpers inside expressions, using a throw directly results in much simpler code overall. Though it didn't seem like particularly common occurrence, at least in System.Text.Json.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 17, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 23, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants