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
Fix #8453: [Script] Don't truncate loan variation to 32bit #8456
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mm, bitstuffing. Code looks fine.
src/misc_cmd.cpp
Outdated
* @param p1 amount to decrease the loan with, multitude of LOAN_INTERVAL. Only used when p2 == 2. | ||
* @param p2 when 0: pays back LOAN_INTERVAL | ||
* when 1: pays back the maximum loan permitting money (press CTRL), | ||
* when 2: pays back the amount specified in p1 | ||
* @param p1 higher half of amount to increase the loan with, multitude of LOAN_INTERVAL. Only used when (p2 & 3) == 2. | ||
* @param p2 (bit 2-31) - lower half of amount (lower 2 bits assumed to be 0) | ||
* (bit 0-1) - when 0: loans LOAN_INTERVAL | ||
* when 1: loans the maximum loan permitting money (press CTRL), | ||
* when 2: loans the amount specified in p1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments are about decreasing loan, not increasing loan.
EDIT: Also the amount specified is not only in p1 anymore
@@ -204,8 +204,10 @@ | |||
|
|||
if (loan == GetLoanAmount()) return true; | |||
|
|||
Money amount = abs(loan - GetLoanAmount()); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert(amount >> 62 == 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that suggestion is totally wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about
assert(GB((int64)amount, 62, 2) == 0);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking highest 2 bits is useless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am guessing he was trying to ensure the lower (not higher) bits were zero'd. This is pointless, as we already check that the modulo of the interval is zero, and the interval is validated to have its lower bits zero. And yes, you can make code too pedantic :D
@@ -204,8 +204,10 @@ | |||
|
|||
if (loan == GetLoanAmount()) return true; | |||
|
|||
Money amount = abs(loan - GetLoanAmount()); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am guessing he was trying to ensure the lower (not higher) bits were zero'd. This is pointless, as we already check that the modulo of the interval is zero, and the interval is validated to have its lower bits zero. And yes, you can make code too pedantic :D
Motivation / Problem
Difference between requested new loan and current loan may not fit in 32 bits.
Description
Replaced p1 with higher half of loan variation.
Used free bits in p2 to pass 30 highest bits of lower half of loan variation (assuming lower 2 bits are always 0)
Limitations
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.