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

Prevent item::charges_per_weight to return overflowed values #79422

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

marilynias
Copy link
Contributor

@marilynias marilynias commented Jan 30, 2025

Summary

Bugfixes "Prevent item::charges_per_weight to return overflowed values"

Purpose of change

fix #79414
item::charges_per_weight and item::charges_per_volume didnt have any overflow protection. Thus, wen trying to put an item on the ground with AIM (practically infinite weight capacity) item::charges_per_weight would try to return a number greater then INT_MAX.

Describe the solution

partially rewrite item::charges_per_weight to de-duplicate code and return min(items, INFINITE_CHARGES ).

Describe alternatives you've considered

return int64_t instead of int

Testing

Can drop items again

Additional context

If only we could unittest AIM.

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` <Bugfix> This is a fix for a bug (or closes open issue) labels Jan 30, 2025
marilynias and others added 4 commits January 30, 2025 11:27
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot added json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions and removed astyled astyled PR, label is assigned by github actions labels Jan 30, 2025
@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label Jan 30, 2025
@RenechCDDA
Copy link
Member

The approach seems reasonable. Assuming tests pass I like the solution.

If only we could unittest AIM.

You could theoretically break up AIM's functions enough to make a test for it. That would probably be a large-ish refactoring though, so not in scope. If you're interested and need to know more, I am happy to explain.

(Also it looks like you re-used the branch for your last PR? Not sure if that was intentional. Just FYI)

@marilynias
Copy link
Contributor Author

marilynias commented Jan 30, 2025

(Also it looks like you re-used the branch for your last PR? Not sure if that was intentional. Just FYI)

yup, not intentional, thats why I rebased

@marilynias marilynias marked this pull request as draft January 30, 2025 14:10
@marilynias marilynias marked this pull request as ready for review January 30, 2025 14:11
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 30, 2025
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` json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Advanced Inventory Management not allowing some things to be dropped
2 participants