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

Add inline annotations and avoid 128-bit where it's not needed in TryFrom + tests for xsize/x128 #43194

Closed
wants to merge 4 commits into from

Conversation

oyvindln
Copy link
Contributor

This should make the output in #43064 slightly cleaner, and hopefully help fix #43124 when combined with #43127. I think the main issue was the lack of inline annotations for try_from (and not the use of 128-bit values as I first thought). Additionally, the implementations of try_from called min/max_value through the FromStrRadixHelper trait, which weren't marked inline either. I would have thought LLVM would be smart enough to inline this anyway, but it seems it isn't. While there has been talk about moving to a Cast trait for number types, it looks like it will take a while before that can happen, so this should help a bit in the meantime.

I didn't do any pointer width specific implementations for now.

This change also makes the assumption that i/usize is not larger than 64 bits wide to simplify things a little. Is that a safe assumption to make for now?

All the tests passed, but it's still possible I could have missed some parts.

This PR:
Avoids using i/u128 except for converting between 128-bit types.
Use the smallest type able to represent the maximum value instead for most types (except u/isize), and i/u64 otherwise.

Add sinline annotations to try_from implementations.
Before doing this the functions did not get inlined.

Avoids using the FromStrRadixHelper trait in the try_from implementations, as the trait wasn't actually needed for anything.

Add some simpler implementations for conversions that don't need as many checks. In release mode the checks will be optimized out when they're not needed anyhow, but this should at least result in very slightly less code non-optimized builds and very slightly less work for LLVM.

…From

Avoid using i/u128 except for converting between 128-bit types.
   Use the smallest needed type instead for most types (except u/isize)

Add inline annotations to try_from implementations.
   Before doing this the functions did not get inlined.

Avoid using the FromStrRadixHelper trait in the try_from implementations.
   This wasn't needed for anything.

Add some simpler implementations for conversions that don't need as many checks.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@aidanhs
Copy link
Member

aidanhs commented Jul 13, 2017

Thanks for the PR @oyvindln, we'll check in now and again to make sure @aturon or another reviewer gets to this soon!

@aidanhs aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 13, 2017

// The current implementations assume that usize/isize <= 64 bits wide.
Copy link
Member

Choose a reason for hiding this comment

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

This comment should be above over the trivial_try_from_impl!(usize, usize, u128, i128); rather than the whole block.

I would also recommend doing something like

#[cfg(not(any(target_pointer_width="16", ..., width="64")))]
compile_error!("implementation assumes this and that")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense.

trivial_try_from_impl!(i64, i64, i128);
trivial_try_from_impl!(i128, i128);
trivial_try_from_impl!(isize, isize, i128);
trivial_try_from_impl!(usize, u64);
Copy link
Member

Choose a reason for hiding this comment

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

Why separate from the trivial_try_from_impl!(usize, usize, u128, i128); above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, simply an oversight, you're right it should be together with the other unsigned ones.

same_sign_try_from_wider_impl!(u64, u128);
same_sign_try_from_wider_impl!(i64, i128);

// Need to make sure the storage component is large enough for these.
Copy link
Member

Choose a reason for hiding this comment

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

Ditto wrt comment. Use a compile_error! to verify your assumptions.


// Need to make sure the storage component is large enough for these.
// (storage, target, $(source))
same_sign_try_from_int_impl!(u64, usize, u64, u32, u16);
Copy link
Member

Choose a reason for hiding this comment

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

These would be better off with a #[cfg] and trivial_try_from_impl or try_from_wider depending on what the value of target_pointer_width is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially decided to leave that for later as it seemed a bit complex to fully test, but I suppose I can try to do it properly.

@oyvindln
Copy link
Contributor Author

Thanks for the feedback. I will see if I can sort out the issues pointed out.

Moved u64-usize macro call to the logical place
@oyvindln
Copy link
Contributor Author

Okay, I think I sorted the requested changes.

I'm a bit unsure about the "compile_error" macro call after checking for an unknown bit width. I didn't enable the compile_error feature, as that would require me to add it to lib.rs, and add a statement to ignore the warning about unused feature. Though, at least it won't compile if the condition is hit, even though it won't give the proper compile_error message.

I didn't manage to compile for the 16-bit target (I got an missing specification error for the msp430_none_elf target which I don't know how to solve.), so that part is untested still.

I also couldn't find any tests for the usize and u128 types for TryFrom. Maybe that's something that ought to be added.

@SimonSapin
Copy link
Contributor

I didn't manage to compile for the 16-bit target (I got an missing specification error for the msp430_none_elf target which I don't know how to solve.), so that part is untested still.

(Perhaps #43099 has not reached stage 0 yet? Before rust-lang/rust-forge#80 the platform support page linked to https://github.com/pftbest/rust_on_msp/blob/master/msp430.json.)

@oyvindln
Copy link
Contributor Author

oyvindln commented Jul 13, 2017

Yup, that seems to be the issue. I'm trying to build with rustc set to the latest nightly (which does have msp430 support) in config.toml now.

EDIT: So I didn't manage to compile for msp430 using x.py directly, but I did manage to compile libcore with the changes in this commit for msp430 with xargo at least.

@nagisa
Copy link
Member

nagisa commented Jul 14, 2017

#[cfg_attr(any(target_poin...), feature(compile_error))] should work for the conditional feature.

@nagisa
Copy link
Member

nagisa commented Jul 14, 2017

@oyvindln do you want to add the conversion tests for usize & u128? If not, I’ll review it as is.

@oyvindln
Copy link
Contributor Author

oyvindln commented Jul 14, 2017

@nagisa You can review it as is for now. I can do another PR with the tests once this one is sorted.

same_sign_try_from_wider_impl!(u64, u128);
same_sign_try_from_wider_impl!(i64, i128);
same_sign_try_from_wider_impl!(usize, u128);
same_sign_try_from_wider_impl!(isize, u128);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn’t this be isize, i128?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed

trivial_try_from_impl!(isize, i32);
// x64 -> xsize
same_sign_try_from_wider_impl!(usize, u64);
same_sign_try_from_wider_impl!(isize, u64);
Copy link
Member

Choose a reason for hiding this comment

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

A cross-sign conversion again.

I think this code would be easier follow if it was only split into blocks for each target_pointer_width.

Something like:

#[cfg(target_pointer_width = "16")] { 
     trivial_try_from_impl!(usize, u16, u32);
     trivial_try_from_impl!(isize, i16, i32);
     same_sign_try_from_wider_impl!(usize, u32, u64);
     same_sign_try_from_wider_impl!(isize, i32, i64);
}

// and so forth.

Copy link
Member

@nagisa nagisa Jul 14, 2017

Choose a reason for hiding this comment

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

Also, I believe the argument order is swapped for same_sign_try_from_wider_impl here and in the block above? Was looking at the wrong macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It make make it more readable yeah.

@oyvindln
Copy link
Contributor Author

So I think I'm going to try to add tests as well in this PR, looks like it's easy to miss stuff otherwise.

…e readable

Adds tests for the TryFrom impelemtatnions for xsize and x128 values together with the
 existing tests for this trait.

Fix a few impls that had the wrong values, and add some that had been missed in the last
 commits.

Make TryFrom impl macros match on "into/from" to make them a bit more readable, and
 group the arguments on the macros that use a storage value.
@oyvindln oyvindln changed the title Add inline annotations and avoid 128-bit where it's not needed in TryFrom Add inline annotations and avoid 128-bit where it's not needed in TryFrom + tests for xsize/x128 Jul 15, 2017
@oyvindln
Copy link
Contributor Author

oyvindln commented Jul 15, 2017

The last commit adds the missing tests, and hopefully fixes the issues from the last one.

I borrowed the idea of using from/into as a separator in the macros from the try_from crate. Hopefully that makes it a bit easier to follow.

Also there is a new PR #43248 that does something similar to this PR. The approach used there may be a bit cleaner.

@nagisa
Copy link
Member

nagisa commented Jul 16, 2017

I’m personally fine with either approach (they both look fairly similar to me). @llogiq needs to at least add tests and the #[inline] annotations -- something your PR has and their doesn’t.

If you feel that their implementation is better, I would suggest @llogiq to simply take your tests and merge that.

/me shrugs

@oyvindln
Copy link
Contributor Author

If you think this one is fine, feel free to review/merge.

@llogiq
Copy link
Contributor

llogiq commented Jul 16, 2017

@oyvindln are you OK with me cherry-picking your PR?

@oyvindln
Copy link
Contributor Author

@llogiq Yup, that's fine by me.

@aturon
Copy link
Member

aturon commented Jul 17, 2017

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned aturon Jul 17, 2017
@alexcrichton
Copy link
Member

ping r? @nagisa

@carols10cents
Copy link
Member

ping @nagisa @rust-lang/libs, it seems like this is ready for rereview!

@nagisa
Copy link
Member

nagisa commented Jul 24, 2017

See this.

@oyvindln
Copy link
Contributor Author

I think #43248 will be used instead. (Though I can fix up this to add the modules and avoid storage types if you don't feel like waiting for the other PR to be finished.)

@oyvindln
Copy link
Contributor Author

Looks like #43248 is ready now, so I'm closing this in favour of that.

@oyvindln oyvindln closed this Jul 24, 2017
@oyvindln oyvindln deleted the tryfrom_avoid_128 branch July 27, 2017 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collect<Vec<u16>> from range doesn't optimize well.
9 participants