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

Remove as casts. #2562

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

reitermarkus
Copy link
Contributor

Use TryInto instead of as casts.

@emilio
Copy link
Contributor

emilio commented Jun 16, 2023

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.

@reitermarkus
Copy link
Contributor Author

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.

@emilio
Copy link
Contributor

emilio commented Jun 17, 2023

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.

@reitermarkus
Copy link
Contributor Author

reitermarkus commented Jun 17, 2023

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);

@pvdrz
Copy link
Contributor

pvdrz commented Jun 17, 2023

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.

@emilio
Copy link
Contributor

emilio commented Jun 17, 2023

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

@reitermarkus
Copy link
Contributor Author

reitermarkus commented Jun 17, 2023

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().

bindgen/clang.rs Outdated Show resolved Hide resolved
@workingjubilee
Copy link
Member

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.

@bors-servo
Copy link

☔ The latest upstream changes (presumably 5ff913a) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am always a fan of removing as in favor of try_into, even if the error is extremely unlikely to happen.

I don't think that the expect messages add that much compared to unwrap(). Unwrapping already tells you that there is an integer conversion issue and points to the line, not much to gain for the extra effort of writing down the exact types in a message. And a lot of these should literally never fail, as much as I would like to see the struct causing type alignment does not fit in `usize` 🙂

bindgen/clang.rs Outdated Show resolved Hide resolved
bindgen/clang.rs Outdated Show resolved Hide resolved
@reitermarkus
Copy link
Contributor Author

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

@reitermarkus
Copy link
Contributor Author

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

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple notes about a redundant pattern, but this looks a lot better to me than the previous rev. Emilio needs to double check the logic refactoring.

bindgen/clang.rs Show resolved Hide resolved
bindgen/clang.rs Outdated Show resolved Hide resolved
bindgen/clang.rs Outdated Show resolved Hide resolved
bindgen/clang.rs Outdated Show resolved Hide resolved
bindgen/clang.rs Outdated Show resolved Hide resolved
bindgen/clang.rs Outdated Show resolved Hide resolved
bindgen/clang.rs Outdated Show resolved Hide resolved
bindgen/clang.rs Outdated Show resolved Hide resolved
bindgen/clang.rs Outdated Show resolved Hide resolved
bindgen/clang.rs Outdated Show resolved Hide resolved
bindgen/clang.rs Outdated Show resolved Hide resolved
bindgen/clang.rs Outdated Show resolved Hide resolved
bindgen/clang.rs Outdated Show resolved Hide resolved
bindgen/clang.rs Outdated Show resolved Hide resolved
bindgen/clang.rs Outdated Show resolved Hide resolved
@bors-servo
Copy link

☔ The latest upstream changes (presumably 2013b8c) made this pull request unmergeable. Please resolve the merge conflicts.

@reitermarkus
Copy link
Contributor Author

@pvdrz, @emilio, please review this again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants