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

Remove CheckForOverflowUnderflow #63825

Merged
merged 2 commits into from
Jan 29, 2022
Merged

Remove CheckForOverflowUnderflow #63825

merged 2 commits into from
Jan 29, 2022

Conversation

Marusyk
Copy link
Member

@Marusyk Marusyk commented Jan 15, 2022

Remove set false to CheckForOverflowUnderflow due to it is disabled b…y default.

Issue #63725

Please review
Thank you in advance

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 15, 2022
@ghost
Copy link

ghost commented Jan 15, 2022

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

Issue Details

Remove set false to CheckForOverflowUnderflow due to it is disabled b…y default.

Issue #63725

Please review
Thank you in advance

Author: Marusyk
Assignees: -
Labels:

area-Infrastructure-libraries, community-contribution
Milestone: -

safern
safern approved these changes Jan 15, 2022
View reviewed changes
Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

Thanks!

AraHaan
AraHaan approved these changes Jan 15, 2022
View reviewed changes
Copy link
Member

@AraHaan AraHaan left a comment

Choose a reason for hiding this comment

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

Test failure on the 1 item so far looks unrelated to this change.

@stephentoub
Copy link
Member

There are other places in the tree it's set to true as well. Are they needed for some reason?

@AraHaan
Copy link
Member

AraHaan commented Jan 15, 2022 •
edited
Loading

There are other places in the tree it's set to true as well. Are they needed for some reason?

I think it is needed in some places possibly due to some overflow or underflow being intended in some places in the runtime? It's just a guess on why it might be true in those spots or if it's set to true just "in case" something like that happens.

Either way I think it would need some extensive tests to make sure those are not needed first.

@jkotas
Copy link
Member

jkotas commented Jan 16, 2022

There are other places in the tree it's set to true as well

I see exactly one under runtime tests. It can be trivially replaced by enclosing the body of the test in checked context if we do not want to have CheckForOverflowUnderflow in the test csproj.

@stephentoub
Copy link
Member

I see exactly one under runtime tests

Yeah, not sure where I was searching before when I saw pages of results (both false and true), but we might as well just get rid of it entirely.

@deeprobin
Copy link
Contributor

Seems to be a unrelated test (rebasing on main might help)

deeprobin
deeprobin approved these changes Jan 17, 2022
View reviewed changes
Copy link
Contributor

@deeprobin deeprobin left a comment

Choose a reason for hiding this comment

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

LGTM

@Marusyk
Copy link
Member Author

Marusyk commented Jan 26, 2022

Please let me if I also need to remove CheckForOverflowUnderflow from

runtime/src/tests/JIT/Regression/Dev11/dev11_4421/Dev11_4421.csproj

Line 8 in 2cf4826

<CheckForOverflowUnderflow>True</CheckForOverflowUnderflow>

@jkotas
Copy link
Member

jkotas commented Jan 27, 2022

Please let me if I also need to remove CheckForOverflowUnderflow from

Yes, please remove it and add checked block to the test source instead.

stephentoub
stephentoub approved these changes Jan 29, 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 stephentoub merged commit 874d6a3 into dotnet:main Jan 29, 2022
@deeprobin deeprobin mentioned this pull request Jan 30, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Feb 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub approved these changes

@deeprobin deeprobin deeprobin approved these changes

@AraHaan AraHaan AraHaan approved these changes

@safern safern safern approved these changes

Assignees
No one assigned
Labels
area-Infrastructure-libraries community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Marusyk @stephentoub @AraHaan @jkotas @deeprobin @safern