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

Fixed clamping order count in trading inventory #75849

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

l29ah
Copy link
Contributor

@l29ah l29ah commented Aug 20, 2024

Summary

Bugfixes "Fixed clamping order count in trading inventory"

Purpose of change

fixes #75742

Describe the solution

Argument order and types were all wrong. Now they're right.

Describe alternatives you've considered

Keeping the yet-silent UB.

Testing

Tested on the saved game provided in the issue report.

Additional context

Found due to -D_GLIBCXX_ASSERTIONS, consider enabling it for experimental builds maybe?

@github-actions github-actions bot added <Bugfix> This is a fix for a bug (or closes open issue) Info / User Interface Game - player communication, menus, etc. [C++] Changes (can be) made in C++. Previously named `Code` astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Aug 20, 2024
@l29ah
Copy link
Contributor Author

l29ah commented Aug 20, 2024

Ouch, i didn't expect we have non-POSIX targets. Could you advise me on the type to use instead of ssize_t?

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Aug 21, 2024
@andrei8l
Copy link
Contributor

ssize_t doesn't make sense in that context and you haven't justified that change either. Even size_t is questionable, but please stick to just fixing the argument order for this PR.

@l29ah
Copy link
Contributor Author

l29ah commented Aug 21, 2024

Fixing argument order alone won't make it work good since an unsigned integer overflow also happens here.

@andrei8l
Copy link
Contributor

Fixing argument order alone won't make it work good since an unsigned integer overflow also happens here.

Irrelevant because size_t is an unsigned type and both overflow and underflow for unsigned types are well defined and the clamp would still correctly produce a number between 0 and max even if you somehow managed to select 18 quintillion items. Even if that weren't the case, ssize_t is the wrong type - you can think of it as an std::optional<size_t> rather than a sort of size_t than can hold arbitrary negative values.

@l29ah
Copy link
Contributor Author

l29ah commented Aug 21, 2024

Clamping UINT_MAX produces another value than clamping -1, and it results in being unable to deselect items by holding - continuously. intmax_t is another option i guess.

@andrei8l
Copy link
Contributor

You can also just keep it simple and revert d84cb31. It was snuck in a completely unrelated PR and the previous code worked fine AFAICT.

@l29ah
Copy link
Contributor Author

l29ah commented Aug 23, 2024

Yeah, i agree that is a saner solution. @Brambor why the clamp?

@Brambor
Copy link
Contributor

Brambor commented Aug 23, 2024

My bad. It would work with int, I believe. I made a mistake. As they say "use int instead of unsigned in CDDA codebase, you will avoid maintance errors". I made that error that day.

I see, it is not unsigned -1, but wrong order of arguments...

Yeah, revert it. I only made it to be more readable. But it is wrong now, as you report.

Sorry for the trouble I caused by "refactoring".

fixes CleverRaven#75742
Found due to -D_GLIBCXX_ASSERTIONS, consider enabling it for experimental builds maybe?
this reverts d84cb31
@l29ah l29ah force-pushed the fixed-trading-clamping branch from 235d238 to b1dfa05 Compare August 24, 2024 20:07
@l29ah
Copy link
Contributor Author

l29ah commented Aug 24, 2024

force-pushed the reverting commit

@github-actions github-actions bot added BasicBuildPassed This PR builds correctly, label assigned by github actions and removed BasicBuildPassed This PR builds correctly, label assigned by github actions labels Aug 24, 2024
@Maleclypse Maleclypse merged commit 9d00e37 into CleverRaven:master Aug 28, 2024
20 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assertion failure in the trading dialog "!(__hi < __lo)"
4 participants