[CR] Use units::volume instead of full type

[mqrause]

Summary

None

Purpose of change

Just came across this when looking at code and thought I'd clean it up.
It's the only occurence of any of the units not using their alias I found.

Describe the solution

Use alias.

Testing

Item stacks still have their volume.

[mqrause]

Ok, so this breaks the item test because of integer overflow. The volume type uses int, but that occurence uses int64 to avoid that. Is it supposed to be like that? If yes, this can be closed. Though I'm not sure what happens when there's enough charges to actually go beyond maxint ml. (I guess this is prevented by implausability anyway.)

[actual-nh]

If the purpose of it is to avoid the overflow, perhaps a comment in the code would be a good idea?

[mqrause]

Yeah, I can change the PR to do that instead, if I get a confirmation.

[actual-nh]

See this commit, although there was a more recent one by @KorGgenT.

[mqrause]

See this commit, although there was a more recent one by @KorGgenT.

I'm more concerned with the left side. num uses int64 for its value, but after the calculations are done, it just gets passed back to ret which uses int. So the calculations are safe, but the result is not. When keeping the types as they are, it might be better/clearer to switch around the logic to do the division first and then assume the multiplication is safe(ish).

[actual-nh]

Ping: @jbytheway

[jbytheway]

It's divided by stack size before being assigned to ret which should be safe because there are other constraints in play which prevent any stack exceeding 1000L in volume.

Doing the division first would be incorrect, because it would round improperly.

[mqrause]

there are other constraints in play which prevent any stack exceeding 1000L in volume.

That makes me somewhat question the test case that makes this require to be an int64, then. It's testing the overflow safety when it shouldn't get anywhere near those values ever.

[actual-nh]

there are other constraints in play which prevent any stack exceeding 1000L in volume.

That makes me somewhat question the test case that makes this require to be an int64, then. It's testing the overflow safety when it shouldn't get anywhere near those values ever.

Is the 1000L limit intentional, or due to the same sort of thing that limits monster volume/size?

[ZhilkinSerg]

there are other constraints in play which prevent any stack exceeding 1000L in volume.

That makes me somewhat question the test case that makes this require to be an int64, then. It's testing the overflow safety when it shouldn't get anywhere near those values ever.

Is the 1000L limit intentional, or due to the same sort of thing that limits monster volume/size?

Answer could possibly be here - #35411

[jbytheway]

So I think the limit might not be 1000L. At some point, around when these 64-bit integers were originally introduced, it was constructed such that there was an upper limit on any vehicle part size (which was set to match the blazemod cargo dimension), and a lower limit on item volume, such that the maximum stack size in any available volume wouldn't exceed 2^31.

I guess now we have to worry about pocket sizes as well as vehicle part capacities. Someone could cause an error here by creating a very generous bag-of-holding item.