rust-bindgen icon indicating copy to clipboard operation
rust-bindgen copied to clipboard

Remove `as` casts.

Open reitermarkus opened this issue 1 year ago • 13 comments

Use TryInto instead of as casts.

reitermarkus avatar Jun 16 '23 16:06 reitermarkus

Can you elaborate on why is this useful?

This seems less legible and as long as the as behavior is well defined which afaiui it is, I'm not sure it's worth doing this.

emilio avatar Jun 16 '23 21:06 emilio

as behavior is well defined which afaiui it is

I don't think casting between c_uint/c_longlong and usize/u32 is well defined, which seems to be the bulk of these.

reitermarkus avatar Jun 16 '23 23:06 reitermarkus

as behavior is well defined which afaiui it is

I don't think casting between c_uint/c_longlong and usize/u32 is well defined, which seems to be the bulk of these.

https://doc.rust-lang.org/reference/expressions/operator-expr.html#numeric-cast looks pretty well defined to me? So it saturates on truncation and zero/sign-extends properly.

emilio avatar Jun 17 '23 07:06 emilio

I mean, yes, it is clearly defined that integers are truncated, but that's not necessarily what we want.

it saturates on truncation

Pretty sure integers don't saturate on truncation, otherwise something like this would be true:

assert_eq!(100000u32 as u16, u16::MAX);

reitermarkus avatar Jun 17 '23 11:06 reitermarkus

I agree that as can be confusing (specially if we're discussing if truncation or saturation is the right behavior). But having .try_into().unwrap() is just causing bindgen to panic at random places without explanation which will be annoying. I'd rather handle each conversion properly or at least comment why unwrap is fine in those cases.

pvdrz avatar Jun 17 '23 16:06 pvdrz

Most of these are effectively impossible that they fail in any reasonable platform. I don't think c_uint::try_into<usize> can ever panic for example

emilio avatar Jun 17 '23 16:06 emilio

Most of these are effectively impossible that they fail in any reasonable platform.

Probably all of these are impossible on reasonable platforms. This just makes it obvious if that isn't actually the case anymore, e.g. due to refactoring/updating a dependency.

I don't think c_uint::try_into<usize> can ever panic for example

~~We can probably change these cases to just .into(), i.e. just fail to compile on unreasonable platforms.~~

Oh right, From<u32> is not implemented for usize, so we can't just use .into().

reitermarkus avatar Jun 17 '23 19:06 reitermarkus

Most of these are effectively impossible that they fail in any reasonable platform. I don't think c_uint::try_into<usize> can ever panic for example

This is not possible even on several un reasonable platforms. There is nothing in the standard explicitly forbidding uintptr_t or size_t from being smaller than unsigned int, but it is effectively an inexorable conclusion from the design of every platform's data model.

workingjubilee avatar Nov 11 '23 03:11 workingjubilee

:umbrella: The latest upstream changes (presumably 5ff913ab3349db1905cfbbf44a9121cbc223249d) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo avatar Dec 18 '23 09:12 bors-servo

@pvdrz, @emilio, can you have another look at this?

reitermarkus avatar Feb 10 '24 00:02 reitermarkus

@pvdrz, @emilio, please have another look here. Thanks.

reitermarkus avatar Feb 20 '24 04:02 reitermarkus

:umbrella: The latest upstream changes (presumably 2013b8cb52381fce7b72d8ca7ae93d65f0cf7118) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo avatar Apr 27 '24 19:04 bors-servo

@pvdrz, @emilio, please review this again.

reitermarkus avatar May 27 '24 07:05 reitermarkus