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

Removing unreachable code #15281

Merged
merged 2 commits into from
Jun 1, 2023
Merged

Removing unreachable code #15281

merged 2 commits into from
Jun 1, 2023

Conversation

T-Gro
Copy link
Member

@T-Gro T-Gro commented May 31, 2023

  1. The condition isWindows7 && (IntPtr.Size = 8) && isWindows8Plus could never be true - Windows cannot have two versions at the same time. Removing all dead code related to that.

No longer necessary parameter for FsiEvaluationSession.ReportUnhandledExceptionSafe kept in order to keep this public type compatible.

2. UnmanagedProcessExecutionOptions.EnableHeapTerminationOnCorruption only did anything in cases where Environment.Version.Major < 3 . This reflects the CLR version, applies to .NET Framework 3.5 or lower - the compiler no longer targets that.

@T-Gro T-Gro requested a review from a team as a code owner May 31, 2023 11:06
@T-Gro
Copy link
Member Author

T-Gro commented May 31, 2023

/run fantomas

  Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
@github-actions
Copy link
Contributor

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Nice

@abonie
Copy link
Member

abonie commented May 31, 2023

Wait, so regarding 1st point, I would assume there's a bug in Windows 7 and 8 (plus?) because it was supposed to be an or. Pretty sure your PR will not cause a regression, but shouldn't we rather fix the bug? Or is it ok to not care about win7/8?

@T-Gro
Copy link
Member Author

T-Gro commented May 31, 2023

This PR is only removing unreachable code.
If there is a bug, it means it was there unfixed (or rather, with a fix that never applied) for a long time - time long enough to get both Win7 and Win8 out of support.

(

@KevinRansom
Copy link
Member

This is a good change.

@abonie abonie merged commit c4ad09a into main Jun 1, 2023
@T-Gro T-Gro deleted the remove-dead-old-code branch June 7, 2023 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants