Add snapped to integer vectors

[joriskleiber]

This adds snapped for integer vectors based on this code.

[joriskleiber]

a % step won't panic because if a is ever equal to i32::MIN(-2147483648) and step is -1, value needs to be -2147483647.5 which is imposible.

[lilizoey]

if value is i32::MIN and step is -1 then a is i32::MIN since -1 / 2 = 0 for i32.

[Bromeon]

Thank you!

Could you also add tests for zero steps? (Either one component 0, or the whole step vector).

[Bromeon]

#[inline] is mostly needed for cross-crate inlining (because Rust simply can't do it without that attribute), but I wouldn't use it for private/crate-local functions. The compiler (or LLVM) should be able to judge whether to inline such functions itself.

[Bromeon]

Additionally, the snap_one method is now duplicated for all vectors. I think it would be better to declare it once manually outside the macro.

[lilizoey]

#[inline] is mostly needed for cross-crate inlining (because Rust simply can't do it without that attribute)

Not entirely true anymore, rust will automatically inline small functions as of 1.75 rust-lang/rust#116505

[Bromeon]

Yeah, this is however hardly useful in practice -- even if a function is "simple", there's a chance that refactoring makes it non-simple and causes pessimizations. As such, always writing #[inline] when inlining is allowed is still the better practice...

But it's a start, I hope this extended in the not too distant future 🙂

[joriskleiber]

I don't know if there is a better way to handle the possible division overflow, but I thinks its better to panic with a message then just letting it crash.

[Bromeon]

Is only this a problem? step == -1 would work?

[joriskleiber]

No, I just realized how many overflow errors this function has... I have no idea how to handle all these, maybe you could give some guidance what the best way forward would be?

[Bromeon]

Limit the input range to the common usage (and panic for others) would be a start. Upon user request, we can then expand in the future.

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-768

[joriskleiber]

So I did the math (hopefully correct) and came up with these, in my opinion reasonable, panic conditions. I hope I didn't miss any other panics or overflows.

[Bromeon]

Thanks a lot for approaching this so carefully! Turns out the seemingly simple snapped is very involved 😮

PR looks good, just some small doc nitpicks and it should be ready!

[Bromeon]

Suggested change

	assert!(
	value != i32::MIN || step >= 0,
	"snapped() called on vector component i32::MIN with step component smaller then 0"
	);
	assert!(
	value != i32::MAX || step <= 1,
	"snapped() called on vector component i32::MAX with step component greater then 1"
	);
	assert!(
	value != i32::MIN || step >= 0,
	"snapped() called on vector component i32::MIN with step component less than 0"
	);
	assert!(
	value != i32::MAX || step <= 1,
	"snapped() called on vector component i32::MAX with step component greater than 1"
	);

[Bromeon]

Maybe this could be worded in a way makes clear that panics only happen in under/overflow scenarios, not general usage and especially not 0 steps:

Suggested change

	/// # Panics
	/// If any component of `self` is `i32::MIN` while the same component on `step` smaller then `0` or
	/// if any component of `self` is `i32::MAX` while the same component on `step` greater then `1`.
	/// # Panics
	/// On under- or overflow:
	/// - If any component of `self` is `i32::MIN` while the same component on `step` less than `0`.
	/// - If any component of `self` is `i32::MAX` while the same component on `step` greater than `1`.

[joriskleiber]

As expected there where more possible overflow scenarios. For whatever reason i thought addition could only overflow for i32::MIN - 1 or i32::MAX + 1.

The description for the second panic condition is horrific but I don't know how to word it otherwise.

[Bromeon]

Thanks for working out the conditions so carefully!

I think we might need to keep an eye on the performance; with all the overflow checks, it could happen that the happy path is now unnecessarily slow. But if this becomes a problem, we can probably use debug_assert! or similar.

[Ughuuu]

I second them becoming debug_asserts, but probably would need to be a bigger change over more things.

[Bromeon]

Thanks!