Skip to content

Revised again: Added TotalEq/TotalOrd/Clone functionality and related tests to B-tree #11263

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

Merged
merged 1 commit into from
Jan 7, 2014

Conversation

niftynif
Copy link
Contributor

@niftynif niftynif commented Jan 1, 2014

Apologies for junking up the feed with all of these separate pull requests. I'm still getting the hang of git and will hopefully be doing less of this nonsense soon. I opened up another PR and closed the one from earlier today because the first PR was coming from the wrong branch of my repo.
Anyway, this contains a fleshed-out implementation of TotalEq/TotalOrd/Clone/ToStr for the whole B-tree structure and relevant tests, integrating suggestions and comments from several community members.
r? @catamorphism

(when (looking-at "[[:space:]]")
(forward-word 1)
(backward-word 1))
(when (looking-at "[[:space:]]") (forward-to-word 1))
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this intentional? Looks like this might have been an accidental revert.

@brson
Copy link
Contributor

brson commented Jan 4, 2014

Howdy, @niftynif.

This history is pretty gnarly, with all the merges and seemingly duplicate commits. Can you try to rebase it and squash it into a single commit to make it easier to understand?

I would start by doing a git pull mozilla (replace 'mozilla' with whatever the name you have for the upstream repo is), checking out your branch and doing a rebase mozilla/master, see if that automatically removes some of the duplication. Then rebase mozilla/master -i to do an interactive rebase. Leave the first 'pick' command intact, then replace all the subsequent 'pick's with 's' (for 'squash'). If that succeeds you should end up with a single commit on top of the current master. Then do a force push back to your 'btree' branch with -f; the pull request will automatically update. It will be a bit easier to review that way and the history will be easier to understand.

I haven't looked closely at the logic here since I am that familiar with B-Trees but if nobody else picks it up over the weekend I'll try to take a crack at it after your rebase.

@niftynif
Copy link
Contributor Author

niftynif commented Jan 4, 2014

Hey @brson,
Thanks for the git advice. I've been struggling with git lately and it's good to hear some step-by-step solutions (both from you and @catamorphism). Hopefully in the future I can avoid these problems altogether, but for now, I'll fix it up.

Tim went over the content of the code with me, so we should be set on that front. I have a few suggestions from him to implement in the next day or so, and I plan to get on that as soon as I can. I wrote down a series of revisions to make quickly (many having to do with style).

Thanks again for your help!

@niftynif
Copy link
Contributor Author

niftynif commented Jan 5, 2014

New commits: I opted to keep the [allow(unused_variable)] because it will be taken out once the insertion methods become more than stubs, which will hopefully be on my next PR.

@catamorphism
Copy link
Contributor

Hey @niftynif -- this looks good, but as @brson said, we should really make sure the commit history looks sensible before merging. It's tidier to not have the patch removing the .el file and then the one restoring it at all than to have the two patches in the history, even if the net effect is correct. This should be achievable with git rebase. I'll drop you a line with more details about that, but wanted to make it clear to others that I'm looking at this :-)

Equals is now compact and uses vec's equals method.  Cmp compares
all elements on branches and leaves (Nodes).
@catamorphism
Copy link
Contributor

Hooray! I'm going to review this.

@niftynif
Copy link
Contributor Author

niftynif commented Jan 7, 2014

I'm digging into Bors' logs to see what happened but I'm not really sure what is going on here. Is there anyone who can help me figure it out? I made sure I had a working build before I submitted the PR so I'm a little confused.

bors added a commit that referenced this pull request Jan 7, 2014
Apologies for junking up the feed with all of these separate pull requests.  I'm still getting the hang of git and will hopefully be doing less of this nonsense soon.  I opened up another PR and closed the one from earlier today because the first PR was coming from the wrong branch of my repo.
Anyway, this contains a fleshed-out implementation of TotalEq/TotalOrd/Clone/ToStr for the whole B-tree structure and relevant tests, integrating suggestions and comments from several community members.
r? @catamorphism
@brson
Copy link
Contributor

brson commented Jan 7, 2014

@niftynif It was a spurious failure, unrelated to your patch. We'll just retry.

@bors bors closed this Jan 7, 2014
@bors bors merged commit 20ccfde into rust-lang:master Jan 7, 2014
@niftynif niftynif deleted the btree branch January 7, 2014 20:15
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 31, 2023
[`arithmetic_side_effects`] Fix rust-lang#11262

Fix rust-lang#11262

Rustc already handles paths that refer literals -> https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=d795058a2e1634c867288c20ff9432c8

```
changelog: [`arithmetic_side_effects`]: Ignore paths that refer literals
```
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.

5 participants