Skip to content

Added cmp/to_str/clone functionality and related tests to B-tree #11051

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

Closed
wants to merge 36 commits into from

Conversation

niftynif
Copy link
Contributor

Filled out the B-tree data structure I'm working on to include more basic functionalities. r? @catamorphism

@@ -14,6 +14,8 @@
//! Starting implementation of a btree for rust.
//! Structure inspired by github user davidhalperin's gist.

#[allow(attribute_usage)];
#[feature(globs)];
Copy link
Member

Choose a reason for hiding this comment

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

feature(globs) won't work here; it's only recognised as a crate-level attribute.

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 actually didn't add [feature(globs)]. I've been trying to work around it. What should I be doing instead?

@brson
Copy link
Contributor

brson commented Dec 18, 2013

Thanks, @niftynif :squirrel:

match *self{
LeafNode(ref leaf) => {
match *other{
LeafNode(ref leaf2) => {
Copy link
Member

Choose a reason for hiding this comment

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

Stylistically, this could be

match *self {
    LeafNode(ref leaf) => {
        match *other {
            LeafNode(ref leaf2) => leaf.cmp(leaf2),
            BranchNode(_) => Less
        }
    }
    BranchNode(ref branch) => {
        match *other {
            BranchNode(ref branch2) => branch.cmp(branch2),
            LeafNode(_) => Greater
        }
    }
}

@niftynif
Copy link
Contributor Author

Thanks again for all the feedback. I am currently working on implementing the changes. #[derived(...)] doesn't seem to be working--it seems like I have to implement the traits manually, but I'm reorganizing things so the traits are more intuitive--impl<K: TotalOrd, V> Clone for BTree<K, V> { }, etc. I'll try to push a new commit sometime tomorrow with all the specified changes. #[feature(globs)] also seems to be entirely necessary, and does not seem to harm anything when I leave it in.

@huonw
Copy link
Member

huonw commented Dec 20, 2013

#[derived(...)] doesn't seem to be working

It needs to be deriving, not derived, e.g.

#[deriving(Clone, Eq, Ord)]
enum Node<K, V> {
    LeafNode(Leaf<K, V>),
    BranchNode(Branch<K, V>)
}

@niftynif
Copy link
Contributor Author

I had it right in my code. It didn't work.

@huonw
Copy link
Member

huonw commented Dec 20, 2013

Any more details? You'll need to have the trait impls for all the there types at once, i.e. every struct and enum will need #[deriving(...)], because each one will be using the impls of the types they contain.

@niftynif
Copy link
Contributor Author

Yeah, that's what I tried--I gave every struct and enum the deriving tag for all of the traits you specified, but it didn't seem to work. Unfortunately, in my haste, I did not save the error message I was getting, but I got a lot of messages like "Failed to find implementation of trait" for V, for which I took away the trait specifications. It seemed like, at the time, I needed to implement everything manually, because putting #[deriving] on everything kept giving me errors.

@niftynif
Copy link
Contributor Author

Oof. Wow. Got some stuff from the origin branch and committed initial changes (which don't address all of the feedback!) so I can show this progress to @catamorphism tonight. Feel free to comment if you want. If it looks like I'm seriously off track let me know. I'm basically in the process of manually implementing all of the relevant traits, but I don't want to get too far if that's not what I should be doing. #[deriving(...)] does not seem to work here (is the vector at fault?)--I tried to add it to every struct and enum and the traits didn't seem to "take."

@niftynif
Copy link
Contributor Author

Most things resolved, just a few touch-ups here and there need to happen (changing the patterns on to_str, organizing the order of the trait implementations, style concerns, comments, etc). Will get to that very soon!

@catamorphism
Copy link
Contributor

FYI, I did a code review with @niftynif over skype, so nobody else needs to review this right now, though of course feel free to point out specific things. (Also, @niftynif will rebase this and re-push soon, after which I'll approve if all goes well.)

@emberian
Copy link
Member

@niftynif looks like you did a bad rebase/merge, too, so make sure to fix that

Wrote some tests for the compare/clone/to_str methods.

Took out doc comments around tests.

Don't allow impls to force public types

This code in resolve accidentally forced all types with an impl to become
public. This fixes it by default inheriting the privacy of what was previously
there and then becoming `true` if nothing else exits.

Closes rust-lang#10545

Committing to show work in progress.  Everything is still messy,
but I want to take this opportunity to get feedback from relevant
parties if I can.

Recommitting after a bad commit that unnecessarily changed files.
@niftynif
Copy link
Contributor Author

Everything is now neat and tidy--hopefully I don't mess it all up while I make serious revisions! Expect new/improved material soon.

lucab and others added 10 commits December 27, 2013 09:49
This commit uniforms the short title of modules provided by libstd,
in order to make their roles more explicit when glancing at the index.

Signed-off-by: Luca Bruno <[email protected]>
I forgot to add this back in after I removed can_resched and then realized I had
to add it back.
…r=cmr

I forgot to add this back in after I removed can_resched and then realized I had
to add it back.
…xcrichton

Some people have requested this, and I think it's quite useful to have
documentation for the compiler libraries. libnative and libgreen are
self-explanatory I think.
#[ author = "Jane Doe" ]; raises "warning: unknown crate attribute"
Uniform the short title of modules provided by libstd, in order to make their roles more explicit when glancing at the index.
lilyball and others added 15 commits December 29, 2013 13:27
This method is primarily intended to allow for converting a [T, ..N] to
a &mut [T].
Turns out with an argument of 0 the function always returns immediately!

Closes rust-lang#11003
Turns out when you grab an OS mutex, you need to be careful about when and where
things are scheduled!
Turns out when you grab an OS mutex, you need to be careful about when and where
things are scheduled!

I've confirmed that I could fairly reliably get a deadlock (1 in 100 times ish) before this, and I cannot get a deadlock after this (after 1000+ runs).

Closes rust-lang#11200
No longer need to handle GNUEABIHF and hard floats since LLVM should now choose the right default. Fixes rust-lang#11164.
Currently any line starting with `#` is filtered from the output,
including line like `#[deriving]`; this patch makes it so lines are only
filtered when followed by a space similar to the current behaviour of
the tutorial/manual tester.
…ichton

* Pass `&ExtCtxt` instead of `@ExtCtxt`.
* Stop passing duplicate parameters around in `expand`.
* Make `ast_fold` methods take `&mut self`.

After these, it should be possible to remove the `@mut` boxes from `ExtCtxt` altogether, though rust-lang#11167 is doing some of that so I'm holding off on that for now. This will probably conflict with that PR, so I'm guessing that one will have to be rebased on top of the other.

r? @pcwalton
use super::*;
>>>>>>> Committing standard compare/to_str/clone features, but no tests yet.
=======
>>>>>>> Don't allow impls to force public types
Copy link
Contributor

Choose a reason for hiding this comment

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

Rebase conflicts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, thanks for pointing this out! I am pretty sure I removed this but have yet to push the relevant commit. Do you know why this weird stuff happens? Obviously I am still very new to git and don't quite have a handle on it yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well that is because you merged upstream commits and there is one that modifies the same lines of code as you did, normally git says "merge conflict" and adds those arrows, like so:

<<<<<<<< HEAD
foo
========
bar
>>>>>>>> merged commit

Where foo is your own code and bar is the remote code. Then you have to merge manually ie. determine what should be kept from both sides.
After you are done you usually cleanup the arrows and equals signs then git add and git commit.

You can find more info on that here: http://githowto.com/resolving_conflicts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, thanks for the explanation and the link to the resource.

bors and others added 10 commits December 30, 2013 01:41
This method is primarily intended to allow for converting a [T, ..N] to
a &mut [T].
Currently any line starting with `#` is filtered from the output,
including line like `#[deriving]`; this patch makes it so lines are only
filtered when followed by a space similar to the current behaviour of
the tutorial/manual tester.
#[ author = "Jane Doe" ]; raises "warning: unknown crate attribute"
replace `pkgid` by `crate_id`
add `comment`
…g, r=pcwalton

Turns out with an argument of 0 the function always returns immediately!

Closes rust-lang#11003
Wrote some tests for the compare/clone/to_str methods.

Took out doc comments around tests.

Don't allow impls to force public types

This code in resolve accidentally forced all types with an impl to become
public. This fixes it by default inheriting the privacy of what was previously
there and then becoming `true` if nothing else exits.

Closes rust-lang#10545

Committing to show work in progress.  Everything is still messy,
but I want to take this opportunity to get feedback from relevant
parties if I can.

Recommitting after a bad commit that unnecessarily changed files.
@niftynif
Copy link
Contributor Author

Ugh...apologies. I seemed to have messed things up again. I will fix everything shortly.

@niftynif niftynif closed this Dec 30, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 2, 2023
Make `eq_op` suggest `.is_nan()`

changelog: Enhancement: [`eq_op`]: Suggests `is_nan()` for `x != x` where `x` is a float
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.