Skip to content

Comments

ArgOutOfRangeException throw helpers#78222

Merged
stephentoub merged 17 commits intodotnet:mainfrom
hrrrrustic:issue-69590
Dec 1, 2022
Merged

ArgOutOfRangeException throw helpers#78222
stephentoub merged 17 commits intodotnet:mainfrom
hrrrrustic:issue-69590

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 😄

@ghost
Copy link

ghost commented Nov 11, 2022

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 new-api-needs-documentation community-contribution Indicates that the PR has been added by a community member labels Nov 11, 2022
@ghost
Copy link

ghost commented Nov 11, 2022

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.

danmoseley
danmoseley reviewed Nov 11, 2022
View reviewed changes
@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

@stephentoub stephentoub Nov 11, 2022

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

@stephentoub stephentoub Nov 11, 2022

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

@stephentoub stephentoub Nov 15, 2022

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

@hrrrrustic hrrrrustic Nov 24, 2022

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

@stephentoub stephentoub Nov 28, 2022

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

danmoseley commented Nov 23, 2022

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

danmoseley
danmoseley reviewed Nov 23, 2022
View reviewed changes
danmoseley
danmoseley reviewed Nov 23, 2022
View reviewed changes
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 reviewed Nov 24, 2022
View reviewed changes
stephentoub
stephentoub reviewed Nov 24, 2022
View reviewed changes
stephentoub
stephentoub reviewed Nov 24, 2022
View reviewed changes
stephentoub
stephentoub reviewed Nov 24, 2022
View reviewed changes
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

hrrrrustic commented Nov 24, 2022

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

stephentoub commented Nov 28, 2022

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/DateTime.cs Outdated
{
throw new ArgumentOutOfRangeException(null, SR.ArgumentOutOfRange_FileTimeInvalid);
}
ArgumentOutOfRangeException.ThrowIfNegative(ticks, null);
Copy link
Member

@stephentoub stephentoub Nov 28, 2022

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.

@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

@stephentoub stephentoub stephentoub approved these changes

@danmoseley danmoseley danmoseley left review comments

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