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.

Sign up for GitHub

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

Jump to bottom

ArgOutOfRangeException throw helpers #78222

Merged
merged 17 commits into from
Dec 1, 2022
Merged

ArgOutOfRangeException throw helpers #78222

merged 17 commits into from
Dec 1, 2022

Conversation

hrrrrustic
Copy link
Contributor

@hrrrrustic hrrrrustic commented Nov 11, 2022 •
edited
Loading

Contribute to #69590, add a few replacements to this PR for root System namespace in corelib

No idea how to write better tests for this 😄

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 11, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Nov 11, 2022

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Contribute to #69590, add a few replacements to this PR for root System namespace in corelib

No idea how to write better tests for this 😄

Author: hrrrrustic
Assignees: -
Labels:

area-System.Runtime, new-api-needs-documentation, community-contribution
Milestone: -

stephentoub
stephentoub reviewed Nov 11, 2022
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/ArgumentOutOfRangeException.cs Outdated
Comment on lines 124 to 125
ThrowIfNegative(value, paramName);
ThrowIfZero(value, paramName);
Copy link
Member

Choose a reason for hiding this comment

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

This will result in two Throw calls at the call site. Along with my previous comment about keeping things streamlined, this should be more like:

if (T.IsNegative(value) || T.IsZero(value))
{
    Throw...
}

Copy link
Member

Choose a reason for hiding this comment

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

@tannergooding, you were suggesting these methods not be constrained to IComparabe and to use IsNegative and IsZero instead... what about these combined scenarios? Currently it seems like the JIT doesn't do a great job at combining the comparisons:
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIGYACMhgYQYG8aHunHiUGAWXIAKAJYA7DAzEBKLj07UeKmQDMG4hgF5tDAAwNChGQwA8B+ctXclNmwBUAFlAgB3EbIDcCmwF9fBgDrbkD6JgFBUnEpGSsVO3sxDS0zPX14+0T7HmdXD29AlWDiwMCABygxADdsDBgmciQIhjz3Tx0APgYMF3cGCRg3BgBBKABzAFd8GCkAeUmMObUAJWwJcZgAUQQwGHKMMQgJTx9qPyA=
@EgorBo, is that fixable?

This was referenced Nov 15, 2022
Open
Closed
stephentoub
stephentoub reviewed Nov 15, 2022
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/DateTime.cs Outdated
{
throw new ArgumentOutOfRangeException(null, SR.ArgumentOutOfRange_FileTimeInvalid);
}
ArgumentOutOfRangeException.ThrowIfNegative(ticks, null);
Copy link
Member

Choose a reason for hiding this comment

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

We're losing context in the message here. We need to decide how much we care.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You marked this as resolved, so replacement is fine?
I just thinking that I can revert that one to make this initial PR like more straightforward without any edge cases. We can return here in another separate PR with replacements

Copy link
Member

Choose a reason for hiding this comment

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

I just thinking that I can revert that one to make this initial PR like more straightforward without any edge cases. We can return here in another separate PR with replacements

Ok, thanks.

@danmoseley
Copy link
Member

For these helpers and the similar existing ones, perf is a wash, right? The advantage of using them is code is shorter?

Do we think it would be useful to have an analyzer/fixer that would replace existing code with throw helpers - these and others? I found only #68326

This was referenced Nov 23, 2022
Closed
Closed
@hrrrrustic @danmoseley
Apply suggestions from code review
0067cbd 
Co-authored-by: Dan Moseley <danmose@microsoft.com>
stephentoub
stephentoub approved these changes Nov 24, 2022
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

thanks

@stephentoub
Copy link
Member

stephentoub commented Nov 24, 2022 •
edited
Loading

For these helpers and the similar existing ones, perf is a wash, right? The advantage of using them is code is shorter?

IL is smaller, and with current JIT resulting asm is smaller plus containing method has a resulting better chance of being inlined.

Do we think it would be useful to have an analyzer/fixer that would replace existing code with throw helpers - these and others? I found only #68326

Yes: #68326 (comment)

@build-analysis build-analysis bot mentioned this pull request Nov 24, 2022
Closed
@hrrrrustic
Copy link
Contributor Author

Analyzer/fixer implementation should be done in roslyn-analyzers repo, there is nothing to change more in /runtime, right? I think I can try to implement them 🤔

@stephentoub
Copy link
Member

Analyzer/fixer implementation should be done in roslyn-analyzers repo

Correct.

there is nothing to change more in /runtime, right?

This PR only uses the new helpers in corelib; we want to use them in the rest of the libraries as well. But we can also wait to do so and use it as a test case for the analyzer/fixer.

I think I can try to implement them 🤔

I'm actually going to take it on, as it'll be more comprehensive than just these ArgumentOutOfRangeExceptions. Thanks, though. If you'd like to experiment with writing analyzers, there are a bunch that are approved and waiting for someone to tackle them.
https://github.com/dotnet/runtime/issues?q=is%3Aopen+is%3Aissue+label%3Aapi-approved+label%3Acode-analyzer

stephentoub
stephentoub approved these changes Nov 28, 2022
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

A few remaining nits, otherwise LGTM. Thanks for working on this!

src/libraries/System.Private.CoreLib/src/System/BitConverter.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/DateTime.cs Outdated
{
throw new ArgumentOutOfRangeException(null, SR.ArgumentOutOfRange_FileTimeInvalid);
}
ArgumentOutOfRangeException.ThrowIfNegative(ticks, null);
Copy link
Member

Choose a reason for hiding this comment

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

I just thinking that I can revert that one to make this initial PR like more straightforward without any edge cases. We can return here in another separate PR with replacements

Ok, thanks.

src/libraries/System.Private.CoreLib/src/System/String.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/String.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/String.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/String.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/String.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/String.cs Show resolved Hide resolved
src/libraries/System.Runtime/tests/System/ArgumentOutOfRangeExceptionTests.cs Outdated Show resolved Hide resolved
src/libraries/System.Runtime/tests/System/ArgumentOutOfRangeExceptionTests.cs Show resolved Hide resolved
@stephentoub stephentoub mentioned this pull request Nov 29, 2022
Merged
@stephentoub stephentoub merged commit ab35b89 into dotnet:main Dec 1, 2022
@stephentoub stephentoub mentioned this pull request Dec 5, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Jan 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@danmoseley danmoseley danmoseley left review comments

@stephentoub stephentoub stephentoub approved these changes

Assignees
No one assigned
Labels
area-System.Runtime community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hrrrrustic @danmoseley @stephentoub @dakersnar