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

Remove CheckForOverflowUnderflow #63725

Closed
deeprobin opened this issue Jan 13, 2022 · 6 comments
Closed

Remove CheckForOverflowUnderflow #63725

deeprobin opened this issue Jan 13, 2022 · 6 comments
Labels
area-Infrastructure-libraries help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@deeprobin
Copy link
Contributor

I think this can be removed

<!-- We decided to keep this disabled by default: https://github.com/dotnet/runtime/issues/15152 -->
<CheckForOverflowUnderflow>false</CheckForOverflowUnderflow>
because #15152 is closed

@dotnet-issue-labeler
Copy link

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

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jan 13, 2022
@ghost
Copy link

ghost commented Jan 13, 2022

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

Issue Details

I think this can be removed

<!-- We decided to keep this disabled by default: https://github.com/dotnet/runtime/issues/15152 -->
<CheckForOverflowUnderflow>false</CheckForOverflowUnderflow>
because #15152 is closed

Author: deeprobin
Assignees: -
Labels:

area-Infrastructure-libraries, untriaged

Milestone: -

@safern
Copy link
Member

safern commented Jan 13, 2022

Thanks, @deeprobin I think we are referencing that issue on the code as a historical context on why we disabled it globally. We tried to enable it for some time but then disabled it. The comment says we decided to keep it disabled by default, @stephentoub do you happen to know why did we decide to turn this off again?

@stephentoub
Copy link
Member

Because we don't want it enabled. The vast majority of code expects it to be off and it would be a sheer ton of busy work to reliability find and change all the places that depend on it. And at that point, you've either cluttered up the code with lots of ifdefs to handle only having it enabled in debug, or you've added run-time cost to also have it enabled in release. We don't want any of that :)

That said, the default is false, so I'm not sure why it's needed in the Directory.Build.props, unless we're concerned about it being flipped somewhere higher up.

@safern
Copy link
Member

safern commented Jan 13, 2022

Thanks, @stephentoub :).

From a quick GH search for the property that is the only place we have it set, so there is nowhere higher up flipping it on: https://github.com/dotnet/runtime/search?q=CheckForOverflowUnderflow

I think we can safely remove it.

@safern safern added help wanted [up-for-grabs] Good issue for external contributors and removed untriaged New issue has not been triaged by the area owner labels Jan 13, 2022
@safern safern added this to the Future milestone Jan 13, 2022
@deeprobin
Copy link
Contributor Author

I think we can close this issue, because #63825 was merged.

Similar cases are superseded by #63902.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-libraries help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

4 participants