Uh oh!
There was an error while loading. Please reload this page.
Feature/nth back chunks - #60545
Conversation
wizAmit
commented
May 4, 2019
- implemented nth_back for chunks
- added test cases as well
rust-highfive
commented
May 4, 2019
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dtolnay (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. |
| None | ||
| } else { | ||
| let start = match end.checked_sub(self.chunk_size) { | ||
| Some(res) => cmp::min(self.v.len(), res), |
There was a problem hiding this comment.
I think this match arm can just return res? res is guaranteed to be smaller than self.v.len() here. 🙂
There was a problem hiding this comment.
Hey @timvermeulen, thank you for reviewing my PR. And, yes that is true... I'll make the change and commit once all below comments are resolved.
| #[inline] | ||
| fn nth_back(&mut self, n: usize) -> Option<Self::Item> { | ||
| let (end, overflow) = self.v.len().overflowing_sub(n * self.chunk_size); |
There was a problem hiding this comment.
I think n * self.chunk_size could overflow here.
There was a problem hiding this comment.
Yes, that will. However, I was wondering what would the following test result:
let v: &[i32] = &[0, 1, 2, 3];
let mut c = v.chunks(10);
assert_eq!(c.nth_back(1_000_000_000usize), None);
Based on the implementation in this PR, this passes. Should it?? Or, should the result be:
assert_eq!(c.nth_back(1_000_000_000usize), &[0, 1, 2, 3]);
There was a problem hiding this comment.
It should have the same behavior as the default implementation of nth_back, which returns None there.
| let v2: &[i32] = &[0, 1, 2, 3, 4]; | ||
| let mut c2 = v2.chunks(3); | ||
| assert_eq!(c2.nth_back(1).unwrap(), &[0, 1]); |
There was a problem hiding this comment.
This assertion does not currently pass as of 1.36.0-nightly (a340455 2019-05-03). Is there an intentional behavior change in this PR?
There was a problem hiding this comment.
I did not mean to intentionally change behavior with this PR. However, I would try to understand why this should be happening and update.
There was a problem hiding this comment.
c2.nth_back(1).unwrap() should return &[0, 1, 2] here, right? Since that's the first of two chunks, so the second chunk from the back.
There was a problem hiding this comment.
As per the documentation for Chunks: https://github.com/rust-lang/rust/blob/master/src/libcore/slice/mod.rs#L4052-L4056, chunks are non-overlapping, and thereby a chunk of chunk_size 3, should have [ {2,3,4}, {0,1} ] as the slices, for nth_back. Do let me know if I misunderstood anything.
There was a problem hiding this comment.
nth_back is just a method on DoubleEndedIterator, it doesn't actually change the items. [0, 1, 2, 3, 4].chunks(3) returns an iterator over the items &[0, 1, 2] and &[3, 4], in that order, so calling .nth_back(1) on it should return &[0, 1, 2].
Note that your description matches the behavior of rchunks, which does have its first chunk at the end of the slice.
There was a problem hiding this comment.
Hi @timvermeulen I have changed the implementation and pushed the changes... please do let me know if you have any more review comments or any regression fails..
There was a problem hiding this comment.
Looks like something went wrong, the diff is huge now...
There was a problem hiding this comment.
ya this happened i believe because of rebasing to changes in master... shall i post the patch for the changes??
There was a problem hiding this comment.
I'm not really an expert when it comes to git, sorry.
As for your changes to nth – I think it's a lot more complicated than necessary. For starters, you don't need checked_rem because self.chunk_size is guaranteed to be non-zero, and you could replace your usage of checked_sub with saturating_sub.
For inspiration, you could have a looks at my RChunks::nth implementation here, which should probably be pretty similar.
…trochenkov Fixrust-lang#45268 by saving all NodeId's for resolved traits. This fixesrust-lang#45268 by saving all NodeId's for resolved traits. I've verifies this against master (but only on MacOS). However, with some recent changes in master, it appears that there are some failures on my machine. They are unrelated to my changes, anyway.
…hton build dist-aarch64-linux with --enable-profiler This change should enable PGO to be used for aarch64-linux. Fixesrust-lang#57257.
…petrochenkov Fix substs issues for const errors Fixesrust-lang#60503.
…enkov
rustc: collapse relevant DefPathData variants into {Type,Value,Macro,Lifetime}Ns.
`DefPathData` was meant to disambiguate within each namespace, but over the years, that purpose was overlooked, and it started to serve a double-role as a sort of `DefKind` (which rust-lang#60462 properly adds).
Now, we can go back to *only* categorizing namespaces (at least for the variants with names in them).
r? @petrochenkov or @nikomatsakis cc @michaelwoeristerRename `PathResolution` to `PartialRes` Don't use `PartialRes` when `Res` is enough. Rename `Res::kind_name` to `Res::descr` for consistency. Remove `Res::Label`, paths can never resolve to labels. Some further cleanup after rust-lang#60462 r? @eddyb
…li-obk fix Miri visiting generators Fixes fall-out caused by rust-lang#59897. r? @oli-obk
Also updates my entry to the proper name
Keep original literal tokens in AST The original literal tokens (`token::Lit`) are kept in AST until lowering to HIR. The tokens are kept together with their lowered "semantic" representation (`ast::LitKind`), so the size of `ast::Lit` is increased (this also increases the size of meta-item structs used for processing built-in attributes). However, the size of `ast::Expr` stays the same. The intent is to remove the "semantic" representation from AST eventually and keep literals as tokens until lowering to HIR (at least), and I'm going to work on that, but it would be good to land this sooner to unblock progress on the [lexer refactoring](rust-lang#59706). Fixes a part of rust-lang#43081 (literal tokens that are passed to proc macros are always precise, including hexadecimal numbers, strings with their original escaping, etc) Fixes a part of rust-lang#60495 (everything except for proc macro API doesn't need escaping anymore) This also allows to eliminate a certain hack from the lexer (https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/pretty-printing.20comments/near/165005357). cc @matklad
This shouldn't be possible any more, but if it does happen, emit an error rather than maybe panicking later when NLL finds a the ReScope. Impl trait in bindings is sufficiently broken that I don't think this breaks anything that works for it.
Fix HIR printing of existential type rust-lang#60662 This fixesrust-lang#60662
…lacrum syntax: Remove some legacy nonterminal tokens They were used by legacy quote macros removed in rust-lang#51285.
…nkov Assorted cleanup in parser & AST validation r? @petrochenkov Extracted out of a larger PR.
…ywiser Fix minor typos for ItemLocalId * added comma after 'that is' * "can be implement" -> "can be implemented"
Rollup of 4 pull requests Successful merges: - rust-lang#60694 (Fix HIR printing of existential type rust-lang#60662) - rust-lang#60750 (syntax: Remove some legacy nonterminal tokens) - rust-lang#60751 (Assorted cleanup in parser & AST validation) - rust-lang#60752 (Fix minor typos for ItemLocalId) Failed merges: r? @ghost
These will be used in the subsequent commits. Many of them are attributes. The commit also adds the ability to handle symbols that aren't identifiers (e.g. "proc-macro").
Because it's going to be used a lot.
And also the equality between `Path` and strings, because `Path` is made up of `Symbol`s.
…henkov Use `Symbol` more A `Symbol` can be equated with a string (e.g. `&str`). This involves a TLS lookup to get the chars (and a Mutex lock in a parallel compiler) and then a char-by-char comparison. This functionality is convenient but avoids one of the main benefits of `Symbol`s, which is fast equality comparisons. This PR removes the `Symbol`/string equality operations, forcing a lot of existing string occurrences to become `Symbol`s. Fortunately, these are almost all static strings (many are attribute names) and we can add static `Symbol`s as necessary, and very little extra interning occurs. The benefits are (a) a slight speedup (possibly greater in a parallel compiler), and (b) the code is a lot more principled about `Symbol` use. The main downside is verbosity, particularly with more `use syntax::symbol::symbols` items. r? @Zoxc
…pes, r=oli-obk Fix more escaping ReScopes Closesrust-lang#58840
…, r=oli-obk save-analysis: Fix ICE when processing associated constant Closesrust-lang#59134Closesrust-lang/rls#1449 Thanks @swgillespie for helping tracking this down and fixing it! r? @eddyb
impl_trait_in_bindings is not yet complete and can lead to compiler crashes. Fixesrust-lang#60764.
…=pnkfelix Explain error when yielding a reference to a local variable Closesrust-lang#56508
coretest: Downgrade deny to warn The `deny` causes a build failure in https://github.com/RalfJung/miri-test-libstd. Since we use `-D warnings` for rustc builds, `warn` should be enough to lead to compile errors here, without impeding external builds.
…s, r=alexcrichton Add #[doc(hidden)] attribute on compiler generated module. Resolves unavoidable `missing_docs` warning/error on proc-macro crates. Resolvesrust-lang#42008. Changes not yet tested locally, however I wanted to submit first since `rustc` takes forever to compile.
… r=matthewjasper Use `delay_span_bug` for error cases when checking `AnonConst` parent Fixesrust-lang#60704. Fixesrust-lang#60650.
add impl_trait_in_bindings to INCOMPLETE_FEATURES impl_trait_in_bindings is not yet complete and can lead to compiler crashes. Fixesrust-lang#60764.
Rollup of 5 pull requests Successful merges: - rust-lang#60176 (Explain error when yielding a reference to a local variable) - rust-lang#60201 (coretest: Downgrade deny to warn) - rust-lang#60562 (Add #[doc(hidden)] attribute on compiler generated module.) - rust-lang#60710 (Use `delay_span_bug` for error cases when checking `AnonConst` parent) - rust-lang#60770 (add impl_trait_in_bindings to INCOMPLETE_FEATURES) Failed merges: r? @ghost
Signed-off-by: wizAmit <amitforfriends_dns@yahoo.com>
Signed-off-by: wizAmit <amitforfriends_dns@yahoo.com>
dtolnay
left a comment
There was a problem hiding this comment.
Looks like something went wrong with the rebase. Please open a new PR with just the nth_back changes. You will need to rebase the commits onto the most recent master branch. After rebase you should see that git log origin/master..HEAD shows only the commits that you intend to submit for review. Thanks!