8000 [WOW64] Added more missing pieces and the interpreter works by ksco · Pull Request #2682 · ptitSeb/box64 · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[WOW64] Added more missing pieces and the interpreter works #2682

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
Merged
merged 2 commits into from
May 29, 2025
Merged

[WOW64] Added more missing pieces and the interpreter works #2682

merged 2 commits into from
May 29, 2025

Conversation

ksco
Copy link
Collaborator
@ksco ksco commented May 29, 2025

No description provided.

@ksco ksco requested a review from ptitSeb May 29, 2025 07:54
@ksco
Copy link
Collaborator Author
ksco commented May 29, 2025

@AndreRH I ported some more missing part from hangover, please have a look if you find the time. :^)

@AndreRH
Copy link
Collaborator
AndreRH commented May 29, 2025

First of all: I won't always be starting a review as quickly as today :)

Second: I know it's very common here to have squashing and non-atomic commits, but I really don't understand why saving on the commit count would have any benefit over easy reviews, comprehensibility and easy git bisects. So having all those topics in one commit really is a mess to me: various formatting changes, syscall cleanup, signal handling, file moving, adding debug messages. I'd have liked to see at least 5 commits here.

Anyway, I'll leave some comments on the code

AndreRH
AndreRH reviewed May 29, 2025
View reviewed changes
ksco
ksco commented May 29, 2025
View reviewed changes
Copy link
Collaborator Author
@ksco ksco left a comment

Choose a reason for hiding this comment

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

Thanks for the quick review! I've addressed the suggestions, please check again!

@ksco
Copy link
Collaborator Author
ksco commented May 29, 2025

First of all: I won't always be starting a review as quickly as today :)

No problem!

Second: I know it's very common here to have squashing and non-atomic commits, but I really don't understand why saving on the commit count would have any benefit over easy reviews, comprehensibility and easy git bisects. So having all those topics in one commit really is a mess to me: various formatting changes, syscall cleanup, signal handling, file moving, adding debug messages. I'd have liked to see at least 5 commits here.

Well, I don't have opinion on the atomic commits stuff, just being lazy... But I can surely have the commits atomic next time I request a review from you.

Anyway, I'll leave some comments on the code

@AndreRH
Copy link
Collaborator
AndreRH commented May 29, 2025

Seems there's no way for me to mark my comments as resolved.
But now I'd approve this PR

@ptitSeb ptitSeb merged commit 67b15f0 into ptitSeb:main May 29, 2025
28 checks passed
@ksco ksco deleted the wow64 branch May 29, 2025 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndreRH AndreRH AndreRH left review comments

@ptitSeb ptitSeb Awaiting requested review from ptitSeb

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ksco @AndreRH @ptitSeb
0