Crabbake for utils #1926
Crabbake for utils #1926
Conversation
|
For FFI we seem to have pinned |
|
some ffi tests need a compatible llvm, which is not always easy to install for newer rusts. If you can make it work without changing the clang version in the makefiles that should be fine |
|
If you can't, see if it's easy to update to a newer llvm, both on your glinux system and on CI. |
|
As long as all of CI passes, we're fine to update the nightly version. The test that requires compatible LLVM runs on CI now. |
utils/zerovec/src/map/map.rs
Outdated
| @@ -98,6 +98,12 @@ where | |||
| values: V::Container::zvl_new(), | |||
| } | |||
| } | |||
|
|
|||
| #[doc(hidden)] // Crabbake internal | |||
| pub fn from_parts_unchecked(keys: K::Container, values: V::Container) -> Self { | |||
There was a problem hiding this comment.
Discussion: are this and similar constructors better as doc-hidden, doc-hidden unsafe, or pub unsafe?
There was a problem hiding this comment.
I don't think they should be doc-hidden, we shouldn't be special casing CrabBake too much. This one doesn't need to be unsafe but I'm not against making it unsafe anyway.
There was a problem hiding this comment.
I made this doc-hidden because it exposes implementation details. The "parts" it refers to might change in the future.
There was a problem hiding this comment.
oh, that's a fair point i guess. It's unlikely, but makes sense
There was a problem hiding this comment.
How about between safe hidden and unsafe hidden?
I lean toward making these internal constructors unsafe because (1) there are invariants that need to be upheld, (2) people can't accidentally use the functions in an unsafe way, and (3) I see no compelling reason that the methods should not be unsafe
There was a problem hiding this comment.
Oh whoops I meant to make them doc-hidden unsafe.
sffc
changed the title
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
robertbastian
changed the title
| @@ -6,6 +6,9 @@ | |||
|
|
|||
| mod borrowed; | |||
| pub(crate) mod map; | |||
|
|
|||
| #[cfg(feature = "crabbake")] | |||
| mod crabbake; | |||
There was a problem hiding this comment.
suggestion(nb): i'd be okay with all the zerovec types having crabbake impls in one file, the way we do for yoke. either works.
There was a problem hiding this comment.
I've been following serde because I think that's a better metaphor than yoke.
utils/zerovec/src/zerovec/mod.rs
Outdated
| /// # Safety | ||
| /// | ||
| /// `bytes` need to be an output from [`ZeroSlice::as_bytes()`], and `size_of` | ||
| /// is `mem::size_of::<T::ULE>()`. |
There was a problem hiding this comment.
issue: are we not allowed to do that in const yet? bummer.
There was a problem hiding this comment.
I thought we couldn't but we can!
There was a problem hiding this comment.
Yeah I knew the assoc type was a problem in the past but i figured it wouldn't be anymore!
No description provided.