Refactor collections::list #12348

brunoabinader · 2014-02-17T17:17:30Z

This PR includes:

Create an iterator for List<T> called Items<T>;
Move all list operations inside List<T> impl;
Removed functions that are already provided by Iterator trait;
Refactor on len() and is_empty using Container trait;
Bunch of minor fixes;

A replacement for using @ is intended, but still in discussion.

Closes #12344.

brunoabinader · 2014-02-17T17:20:16Z

@alexcrichton r? :-)

bill-myers · 2014-02-17T18:34:28Z

It seems to me any/find/etc. should be just removed, since one can use list.iter().any() and similar instead.

alexcrichton · 2014-02-17T20:27:16Z

src/libcollections/list.rs

@@ -10,170 +10,164 @@

 //! A standard, garbage-collected linked list.

-
+pub type BoxedList<T> = @List<T>;


I'm not sure that this is the right use of a typedef. @List is smaller and more concise to write, and it is also more clear about how the list is built.

The intention here is to reduce usage of @list as I'm planning to replace it with Rc - do you believe it is still worth?

I don't think an alias is necessary here in either case.

Right, removing then.

brunoabinader · 2014-02-21T16:53:02Z

So here goes a rough list of things fixed since first attempt:

Removed iterator-based helper functions when already provided by Iteratortrait;
Removed BoxedList<T> public type (using raw @List<T>instead);
s/BoxedListIterator/Items/;
Removed lifetime parameter 'a;
len() and is_empty() are now provided using Containertrait;
Bunch of test fixes;
Replaced fail!() calls with Option<T> return;
s/push()/unshift()/

brunoabinader · 2014-02-21T17:49:44Z

@alexcrichton, about having iter() for List<T> instead of BoxedListOps<T>, I've created a prototype in https://gist.github.com/brunoabinader/9139223 with some considerations:

On Items<T>, next cannot be @List<T> because &self cannot move out of its dereference in iter(). Because we're using a reference, a lifetime parameter is required. Also, because head in next() cannot move out if its dereference, T deriving Clone is required.

alexcrichton · 2014-02-21T18:25:37Z

src/libcollections/list.rs

        }
-    };
+    }
 }


Hm, I'm not sure how feasible this list will be if it require that its elements be clone-able. This also means that every time you get the length of the list you clone every object in the list!

You're going to want to return &'a T from this iterator so you can remove the Clone bound, which may mean that you need to store &'a List instead of @List like I had originally hoped.

Replacing Option<T> with Option<&'a T> on next()gives the following compilation error because of different signatures between Iterator's trait and this:

/Users/bruno.d/Work/rust/src/libcollections/list.rs:32:5: 40:6 error: method `next` has an incompatible type: expected type parameter but found &-ptr /Users/bruno.d/Work/rust/src/libcollections/list.rs:32 fn next(&'a mut self) -> Option<&'a T> { /Users/bruno.d/Work/rust/src/libcollections/list.rs:33 match *(self.current) { /Users/bruno.d/Work/rust/src/libcollections/list.rs:34 Cons(ref value, ref next) => { /Users/bruno.d/Work/rust/src/libcollections/list.rs:35 self.current = *next; /Users/bruno.d/Work/rust/src/libcollections/list.rs:36 Some(value) /Users/bruno.d/Work/rust/src/libcollections/list.rs:37 },

It does indeed, you'll need to add a lifetime parameter to the struct:

pub struct Items<'a, T> { next: &'a List<T> }

liigo · 2014-02-25T15:28:11Z

iter should be named items, I think
2014年2月25日下午9:06于 "Bruno de Oliveira Abinader" <notifications@github.com

写道：

Fixes since last iteration:

Removed BoxedLisOps: Now all functions are based on List

Removed foldl() in favor of iter().fold()

Removed each() in favor of for x in list.iter()

Make head() use lifetime parameter 'a - avoids clone()

Make len() use iter().len() internally

Bunch of minor fixes

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/12348#issuecomment-36004998
.

brunoabinader · 2014-02-25T15:56:44Z

Fixes since last iteration:

Removed BoxedLisOps: Now all functions are based on List<T>
Removed foldl() in favor of iter().fold()
Removed each() in favor of for x in list.iter()
Make head() use lifetime parameter 'a - avoids clone()
Make len() use iter().len() internally
Bunch of minor fixes (>100 column lines, rebase hell, etc)

brunoabinader · 2014-02-25T15:58:15Z

@liigo the iterator struct is called Items<T>, iter() refers to the function in List<T> that generates them.

brunoabinader · 2014-02-25T19:34:24Z

The build error seems unrelated to this PR:

/home/travis/build/mozilla/rust/src/rustllvm/PassWrapper.cpp:169:47: error: no
      member named 'F_Binary' in namespace 'llvm::sys::fs'
  raw_fd_ostream OS(path, ErrorInfo, sys::fs::F_Binary);
                                     ~~~~~~~~~^

alexcrichton · 2014-02-25T23:40:48Z

src/libcollections/list.rs

-                tl
-            }
-            Nil => return false
+impl<'a, T> List<T> {


Instead of putting the lifetime parameter at the top of the impl block, you can place it on the individual functions below as fn foo<'a>(&'a self) -> ....

You can then merge this impl block with the one below.

alexcrichton · 2014-02-25T23:44:07Z

This is looking great, nice work! Before committing, this'll need to squash some of the commits together, but that can wait until review is done.

bill-myers · 2014-02-26T01:04:32Z

The Nil/Cons construction seems to duplicate Option.

What about defining List equivalently as:

struct Cons<T>
{
    head: T,
    tail: ~Option<Cons<T>>
}

type List<T> = Option<Cons<T>>;

Or ideally with this more natural representation:

struct Cons<T>
{
    head: T,
    tail: Option<~Cons<T>>
}

type List<T> = Option<~Cons<T>>;

(or using a tuple struct instead of with head/tail fields)

Note that also the current representation, equivalent to the first snippet, is very weird, since it starts the list with an inline element, terminates with a ~Nil.

This means that it passes data by value, requiring an explicit copy to prepend an element to the list due to the need to box the Cons and requires an explicit tag for the Nil vs Cons discriminator rather than being able to use the Option null pointer optimization.

It's definitely not how one would naturally implement a singly linked list in C, for example, while the second code snippet in this comment is the natural representation where an empty list is just a null pointer.

eddyb · 2014-02-26T07:10:49Z

This definition should also be possible, for the second snippet above (and I remember mentioning it a couple times in the past, on IRC):

struct Cons<T> {
    head: T,
    tail: List<T>
}
type List<T> = Option<~Cons<T>>;

brunoabinader · 2014-02-26T17:34:06Z

@alexcrichton, @bill-myers, @eddyb: I want to postpone modifying how List<T> is implemented internally until we properly discuss this and agree upon a better choice. What do you guys think?

brunoabinader · 2014-02-26T20:28:49Z

Fixes since last iteration:

Using @alexcrichton's simpler logic for list::from_vec();
Moved lifetime parameter 'a to function signature for list::iter() and list::head();

alexcrichton · 2014-02-26T23:42:27Z

src/librustc/middle/typeck/mod.rs

-        ret
+            if isr_br == br { region = Some(isr_r); break; }
+        };
+        region.unwrap()
    }


This change seems unrelated to the list changes?

Initially I've modified the code to replace list::each() with a for isr in self.iter() statement. But then I ended up with a refactor to simplify get_and_find_region::{get,find}() code. I can separate this if it seems necessary.

alexcrichton · 2014-02-26T23:46:43Z

This looks good to me, I don't think it needs 21 commits, but with some squashings I'll r+

brunoabinader · 2014-02-27T13:02:25Z

Ok, so I squashed a few commits and the # of commits is now down to 18.

…ry, r=alexcrichton This PR includes: - Create an iterator for ```List<T>``` called ```Items<T>```; - Move all list operations inside ```List<T>``` impl; - Removed functions that are already provided by ```Iterator``` trait; - Refactor on ```len()``` and ```is_empty``` using ```Container``` trait; - Bunch of minor fixes; A replacement for using @ is intended, but still in discussion. Closes #12344.

Remove `clippy_utils::get_parent_node` Since it's forwarding to a single method it seems reasonable to use that one directly instead changelog: none

alexcrichton reviewed Feb 17, 2014
View reviewed changes

alexcrichton reviewed Feb 21, 2014
View reviewed changes

alexcrichton reviewed Feb 25, 2014
View reviewed changes

alexcrichton reviewed Feb 26, 2014
View reviewed changes

brunoabinader added 18 commits February 27, 2014 08:31

Replaced @list with ~List in Rust's recursive example

4c553af

Renamed variables

1e6151a

Implement Eq for Cell<T>

8846970

Modified list::from_vec() to return List<T>

2b36276

Implemented Items<'a, T> for List<T>

52524bf

Replaced list::each with iter() in Arenas's Drop impl

0c8731e

Replaced list::each with iter() in get_region

43bc6fc

Removed list::foldl() in favor of iter().fold()

a09a4b8

Removed list::find() in favor of iter().find()

d681907

Removed list::any() in favor of iter().any()

197116d

Implemented list::len() based on Container trait

e589fcf

Implemented list::is_empty() based on Container trait

a14d72d

Removed deprecated list::{iter,each}() functions

223f309

Replaced list::has() with list::contains()

45fd63a

Refactored list::head() to be based on List<T>

fed034c

Refactored list::tail() to be based on List<T>

65f1993

Refactored list::append() to be based on List<T>

1c27c90

Replaced list::push() with unshift() based on List<T>

4da6d04

bors merged commit 4da6d04 into rust-lang:master Feb 28, 2014

adrientetar mentioned this pull request Feb 28, 2014

Should not close pull requests graydon/bors#14

Closed

brunoabinader deleted the libcollections-list-refactory branch February 28, 2014 11:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor collections::list #12348

Refactor collections::list #12348

brunoabinader commented Feb 17, 2014

brunoabinader commented Feb 17, 2014

bill-myers commented Feb 17, 2014

alexcrichton Feb 17, 2014

brunoabinader Feb 17, 2014

alexcrichton Feb 18, 2014

brunoabinader Feb 18, 2014

brunoabinader commented Feb 21, 2014

brunoabinader commented Feb 21, 2014

alexcrichton Feb 21, 2014

brunoabinader Feb 21, 2014

alexcrichton Feb 21, 2014

liigo commented Feb 25, 2014

brunoabinader commented Feb 25, 2014

brunoabinader commented Feb 25, 2014

brunoabinader commented Feb 25, 2014

alexcrichton Feb 25, 2014

brunoabinader Feb 26, 2014

alexcrichton commented Feb 25, 2014

bill-myers commented Feb 26, 2014

eddyb commented Feb 26, 2014

brunoabinader commented Feb 26, 2014

brunoabinader commented Feb 26, 2014

alexcrichton Feb 26, 2014

brunoabinader Feb 27, 2014

alexcrichton commented Feb 26, 2014

brunoabinader commented Feb 27, 2014

		@@ -10,170 +10,164 @@

		//! A standard, garbage-collected linked list.


		pub type BoxedList<T> = @List<T>;

Refactor collections::list #12348

Refactor collections::list #12348

Conversation

brunoabinader commented Feb 17, 2014

brunoabinader commented Feb 17, 2014

bill-myers commented Feb 17, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunoabinader commented Feb 21, 2014

brunoabinader commented Feb 21, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liigo commented Feb 25, 2014

brunoabinader commented Feb 25, 2014

brunoabinader commented Feb 25, 2014

brunoabinader commented Feb 25, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Feb 25, 2014

bill-myers commented Feb 26, 2014

eddyb commented Feb 26, 2014

brunoabinader commented Feb 26, 2014

brunoabinader commented Feb 26, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Feb 26, 2014

brunoabinader commented Feb 27, 2014