Adds impls for more types in std #58
Conversation
remexre
changed the title
|
Rebased this for #57. |
| impl<K: Trace, V: Trace> Finalize for BTreeMap<K, V> {} | ||
| unsafe impl<K: Trace, V: Trace> Trace for BTreeMap<K, V> { | ||
| custom_trace!(this, { | ||
| for (k, v) in this { |
There was a problem hiding this comment.
Why not just this.iter().map(mark)? Shouldn't that work now due to the tuple implementations?
There was a problem hiding this comment.
Iterators are lazy, so I'd need to demand the value at some point; I felt a for-loop would be clearer.
Now that I do have the tuple impls, though, I'll change it to
for pair in this {
mark(pair);
}There was a problem hiding this comment.
Ach, cancel that, there's lifetime issues (this is borrowed, not owned?).
There was a problem hiding this comment.
fair, forgot about that - your original code is better.
gc/src/trace.rs
Outdated
| } | ||
| } | ||
|
|
||
| macro_rules! table_based_finalize_trace { |
There was a problem hiding this comment.
Should this be tuple_based?
There was a problem hiding this comment.
Changed to clarify; it's both tuples and functions, so I was like, "It's a table of types."
I'm renaming it to type_arg_tuple_based_finalized_trace_impls, unless you think that's too long.
| fn_finalize_trace_one!(extern "Rust" fn () -> Ret); | ||
| fn_finalize_trace_one!(extern "C" fn () -> Ret); | ||
| fn_finalize_trace_one!(unsafe extern "Rust" fn () -> Ret); | ||
| fn_finalize_trace_one!(unsafe extern "C" fn () -> Ret); |
There was a problem hiding this comment.
I don't think unsafe extern "C" fn (...) -> Ret will get an impl with this macro.
There was a problem hiding this comment.
Why not? It's "just" a raw C function pointer, right?
There was a problem hiding this comment.
Oh, did you mean literally ... in the sense of varargs? That appears not to be legal.
gc/src/trace.rs
Outdated
| @@ -109,7 +113,71 @@ macro_rules! simple_empty_finalize_trace { | |||
| } | |||
| } | |||
|
|
|||
| simple_empty_finalize_trace![usize, bool, i8, u8, i16, u16, i32, u32, i64, u64, f32, f64, String]; | |||
| simple_empty_finalize_trace![(), isize, usize, bool, i8, u8, i16, u16, i32, u32, i64, u64, f32, f64, char, String, Path, PathBuf]; | |||
There was a problem hiding this comment.
Can you split this over multiple lines?
gc/src/trace.rs
Outdated
| } | ||
|
|
||
| macro_rules! tuple_finalize_trace { | ||
| () => {}; |
There was a problem hiding this comment.
Perhaps add a comment noting that the impl for () is handled in the simple_finalize_empty_trace! invocation above?
| @@ -19,7 +19,7 @@ impl<T: ?Sized> Shared<T> { | |||
| } | |||
|
|
|||
| pub fn as_ptr(&self) -> *mut T { | |||
| self.p.get() | |||
| self.p | |||
There was a problem hiding this comment.
Oops - I pushed a separate fix for this...
|
I still have the questions I listed above for CString, OsString, and LinkedList; I've got a "friendlier" linked list implementation I'd be willing to contribute. Other than that, I'm going to get nightly set up again and check that u/i128 are working, and then I think it's done |
|
With I don't think |
|
Okay, once the unit tests pass, is this good to merge? |
|
Published as |
primitives
()isizechari128u128{i,u}128should be feature-gated behind anightlyfeature.There should be a decision about what the largest tuple arity to support will be -- compiling the impls for each different length it can increase compile times a lot.
For reference,
stdprovides trait impls for up to 31-item arrays and 12-tuples.std::collectionsBTreeMapBTreeSetBinaryHeapHashMapHashSetLinkedListVecDequeDoes it make sense to make a custom singly-linked
GcLinkedListtype instead of or in addition to havingGc<LinkedList>be supported?##std::ffiCStringOsStringThere should probably be some directions with regards to FFI and GC; does e.g.CString::into_rawunroot?std::pathPathPathBufstd::resultResult