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

[RFC] num: bitwise reflection methods #13209

Closed
wants to merge 1 commit into from
Closed

[RFC] num: bitwise reflection methods #13209

wants to merge 1 commit into from

Conversation

lucab
Copy link
Contributor

@lucab lucab commented Mar 30, 2014

A bit-manipulation PR, however I still have some doubts:

  • Library. I believe this is a pretty core operation for several algorithms (eg. many checksums), is the "battery-included" philosophy ok for libstd?
  • Naming. Is reflect() too generic? That's how the operation is usually called, but may be too broad as API and conflict with reflection/introspection.
  • Lot's of redundancy here: 8, 16, 32 and 64 variants. I tried implementing it more generically, however:
    • The Bitwise trait lacks One, Zero and NumCast for this implementation to work. Maybe they should be added anyway?
    • Is it possible to split impl Bitwise for i8..i64 in multiple files? Most of it is through instrinsics in each module, but ideally reflect() could be directly implemented in uint/int_macros abusing $T.

Let Bitwise provide a reflect() method for reversed binary
representation. This is a binary primitive used in several algorithms,
eg. CRC checksums.

Let Bitwise provide a reflect() method for reversed binary
representation. This is a binary primitive used in several algorithms,
eg. CRC checksums.

Signed-off-by: Luca Bruno <[email protected]>
@bill-myers
Copy link
Contributor

The operation for bytes is usually called "byteswap", so maybe this could be called "bitswap", although someone might confuse bits and bytes.

Alternatively, "reverse_bits", or something like that.

Anyway, the implementation in this pull request is terrible; see http://graphics.stanford.edu/~seander/bithacks.html#ReverseParallel for the proper way to do it.

@alexcrichton
Copy link
Member

@bill-myers, please keep the third point of our code of conduct in mind. Blatant insults will not be tolerated.

@lucab
Copy link
Contributor Author

lucab commented Mar 30, 2014

@bill-myers yes, it is a naïve O(n) implementation and it would as well make sense to move to a logarithmic one, trading a bit of clarity/simpllcity. However, as said, I have some other doubts about this PR and it may as well end being scrapped, so it's raw and far from being final.

@alexcrichton
Copy link
Member

How about naming the method bitswap, tagging it #[experimental], and refactoring the implementation to have one shared (but private) function?

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to reopen with a rebase!

matthiaskrgr pushed a commit to matthiaskrgr/rust that referenced this pull request Oct 11, 2022
…Veykril

feat: type inference for generators

This PR implements basic type inference for generator and yield expressions.

Things not included in this PR:
- Generator upvars and generator witnesses are not implemented. They are only used to determine auto trait impls, so basic type inference should be fine without them, but method resolutions with auto trait bounds may not be resolved correctly.

Open questions:
- I haven't (yet) implemented `HirDisplay` for `TyKind::Generator`, so generator types are just shown as "{{generator}}" (in tests, inlay hints, hovers, etc.), which is not really nice. How should we show them?
- I added moderate amount of stuffs to minicore. I especially didn't want to add `impl<T> Deref for &T` and `impl<T> Deref for &mut T` exclusively for tests for generators; should I move them into the test fixtures or can they be placed in minicore?

cc rust-lang#4309
flip1995 pushed a commit to flip1995/rust that referenced this pull request Aug 8, 2024
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.

3 participants