[Draft] Switch to libsolv port #236
Conversation
crates/libsolv_rs/src/id.rs
Outdated
| } | ||
|
|
||
| #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] | ||
| pub struct StringId { |
There was a problem hiding this comment.
Would it make sense to turn this into a tuple struct like the other ids?
crates/libsolv_rs/src/pool.rs
Outdated
| use std::str::FromStr; | ||
|
|
||
| pub struct Pool<'a> { | ||
| pub(crate) solvables: Vec<Solvable<'a>>, |
There was a problem hiding this comment.
I see this usecase a lot in the code. You have a Vec<T> where you intern Ts and return a TId. Accessing the T is done through a TId. This is a typical Arena pattern. It would be nice to have a type to make that more explicit.
You could optimize this by not using a Vec but allocating large chunks on the fly.
See https://github.com/mun-lang/mun/blob/main/crates/mun_hir/src/arena.rs for a simple example.
There was a problem hiding this comment.
Sounds like a good idea! I'll look into it (using a Vec until we have benchmarks, though)
crates/libsolv_rs/src/pool.rs
Outdated
|
|
||
| /// Returns a string describing the last error associated to this pool, or "no error" if there | ||
| /// were no errors | ||
| pub fn last_error(&self) -> String { |
There was a problem hiding this comment.
I would assume this function to return an Option
There was a problem hiding this comment.
I just removed it, since it turns out we weren't even using it (it was a remnant of libsolv)
crates/libsolv_rs/src/pool.rs
Outdated
| "no error".to_string() | ||
| } | ||
|
|
||
| /// Resolves the id to a solvable |
There was a problem hiding this comment.
Returns the solvable right? not an Id? Same for resolve_solvable_mut
There was a problem hiding this comment.
Maybe the wording is confusing... What I mean here is that the function resolves the id into a solvable. I'll reword it in a way that is clearer, something like "returns the solvable associated to the provided id"
| /// = 0: undecided | ||
| /// > 0: level of decision when the solvable is set to true | ||
| /// < 0: level of decision when the solvable is set to false | ||
| map: Vec<i64>, |
There was a problem hiding this comment.
Is this a mapping from a SolvableId to an i64? Might be nice to make a newtype of that?
| use std::marker::PhantomData; | ||
| use std::ops::{Index, IndexMut}; | ||
|
|
||
| /// An `Arena<TValue>` holds a collection of `TValue`s that can be addressed by `TId`s. You can |
There was a problem hiding this comment.
| /// An `Arena<TValue>` holds a collection of `TValue`s that can be addressed by `TId`s. You can | |
| /// An `Mapping<TId, TValue>` holds a collection of `TValue`s that can be addressed by `TId`s. You can |
|
Closing in favor of #243 |
No description provided.