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

Update replay-stocks.sp #1217

Merged
merged 5 commits into from
Sep 23, 2024
Merged

Update replay-stocks.sp #1217

merged 5 commits into from
Sep 23, 2024

Conversation

Nairdaa
Copy link
Contributor

@Nairdaa Nairdaa commented Aug 17, 2024

fix logic behind testing if it works or not and when to actually throw an error

@rtldg
Copy link
Collaborator

rtldg commented Aug 28, 2024

If OpenFile failed then fTest would be null and CloseHandle(null) is okay to do so there's not actually anything broken here although the logic isn't that clear

@Nairdaa
Copy link
Contributor Author

Nairdaa commented Sep 11, 2024

Closing a handle should only be done if it was validly opened. Calling CloseHandle on null may not cause a crash, but it leads to unnecessary function calls.

The "updated code" fixes the "issue" by ensuring that it checks if the file was successfully opened before attempting to close it.

Copy link
Collaborator

@kidfearless kidfearless left a comment

Choose a reason for hiding this comment

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

Looks good, merge it

@rtldg rtldg merged commit 1b67644 into shavitush:master Sep 23, 2024
2 checks passed
@Nairdaa Nairdaa deleted the patch-39 branch September 29, 2024 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants