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

More lua additions #1559

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

More lua additions #1559

wants to merge 2 commits into from

Conversation

johnbaumann
Copy link
Collaborator

@johnbaumann johnbaumann commented Jan 28, 2024

No description provided.

@ghost
Copy link

ghost commented Jan 28, 2024

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

Copy link

codecov bot commented Jan 28, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (a879d9e) 9.77% compared to head (8b77052) 9.78%.

Files Patch % Lines
src/core/pcsxlua.cc 50.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1559   +/-   ##
=======================================
  Coverage    9.77%    9.78%           
=======================================
  Files         441      441           
  Lines      130414   130425   +11     
=======================================
+ Hits        12748    12758   +10     
- Misses     117666   117667    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@johnbaumann johnbaumann marked this pull request as ready for review January 30, 2024 02:18
@nicolasnoble
Copy link
Member

So, I'm in a bit of a pickle here. The cdrom refactoring branch has a change there to make the cycle counter 64 bits. Changing this later would be an API breaking change, but I'm still not ready to merge the cd refactoring one. Might be worth doing a last push to finish it.

@johnbaumann
Copy link
Collaborator Author

So, I'm in a bit of a pickle here. The cdrom refactoring branch has a change there to make the cycle counter 64 bits. Changing this later would be an API breaking change, but I'm still not ready to merge the cd refactoring one. Might be worth doing a last push to finish it.

Would it simplify things if I were to do either/both of the following: Pre-empt the change to the cycle counter and simply change the return type of getCPUCycles() to uint64_t, or, assuming the change is relatively small, piggy back the 64-bit cycle counter change into this PR?

@nicolasnoble
Copy link
Member

So the problem with changing the return type is that the behavior will still be different after the switch: right now, with a 32 bits counter, it'll roll over every ~2 minutes, whereas with a 64 bits counter, it'd never roll over (or rather, will take a few millennias).

And the problem with cherry-picking the change from the cd-rom branch is that the it's a savestate breaking change, and that'd be too many breaking changes in a row, which I'd like to avoid.

@johnbaumann
Copy link
Collaborator Author

So the problem with changing the return type is that the behavior will still be different after the switch: right now, with a 32 bits counter, it'll roll over every ~2 minutes, whereas with a 64 bits counter, it'd never roll over (or rather, will take a few millennias).

And the problem with cherry-picking the change from the cd-rom branch is that the it's a savestate breaking change, and that'd be too many breaking changes in a row, which I'd like to avoid.

I'm ok with putting this pr on ice.

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.

2 participants