alloc: make Cow From impls const#146067
Conversation
|
|
| #[unstable(feature = "bstr", issue = "134915")] | ||
| impl<'a> From<ByteString> for Cow<'a, ByteStr> { | ||
| #[rustc_const_unstable(feature = "const_from", issue = "143773")] | ||
| impl<'a> const From<ByteString> for Cow<'a, ByteStr> { |
There was a problem hiding this comment.
Do we have the equivalent for String? And is ByteString const constructible
There was a problem hiding this comment.
@oli-obk Yes, the equivalent for String is in this same patch.
ByteString is const constructible:
#[repr(transparent)]
pub struct ByteString(pub Vec<u8>);Both Vec and String are const constructible:
https://doc.rust-lang.org/std/vec/struct.Vec.html#method.new
https://doc.rust-lang.org/std/string/struct.String.html#method.new
There was a problem hiding this comment.
Oh lol i stopped scrolling and didnt see the other files
|
@bors r+ |
|
@bors rollup |
alloc: make Cow From impls const This is an expansion of rust-lang#143773 for the `Cow` `From` conversions. r? `@oli-obk`
Rollup of 10 pull requests Successful merges: - #144066 (stabilize c-style varargs for sysv64, win64, efiapi, aapcs) - #145783 (add span to struct pattern rest (..)) - #145961 (resolve: Avoid a regression from splitting prelude into two scopes) - #145962 (Ensure we emit an allocator shim when only some crate types need one) - #146064 (Add compiler error when trying to use concat metavar expr in repetitions) - #146067 (alloc: make Cow From impls const) - #146070 (rustdoc-search: skip loading unneeded fnData) - #146089 (fix a constness ordering bug in rustfmt) - #146094 (Make `Parser::parse_for_head` public for rustfmt usage) - #146102 (Remove dead code stemming from an old effects desugaring) r? `@ghost` `@rustbot` modify labels: rollup
|
Does anyone read the tracking issues any more? Whatever. I explicitly had this as part of #144289 and was not including it because the other reviewer, @tgross35, expressed apprehension about adding heap-allocated trait impls at the end. Part of this PR exists as #145279, which you would find out if you read the comments on the tracking issue you linked. |
alloc: make Cow From impls const This is an expansion of rust-lang#143773 for the `Cow` `From` conversions. r? `@oli-obk`
Rollup of 9 pull requests Successful merges: - #145783 (add span to struct pattern rest (..)) - #145961 (resolve: Avoid a regression from splitting prelude into two scopes) - #145962 (Ensure we emit an allocator shim when only some crate types need one) - #146064 (Add compiler error when trying to use concat metavar expr in repetitions) - #146067 (alloc: make Cow From impls const) - #146070 (rustdoc-search: skip loading unneeded fnData) - #146089 (fix a constness ordering bug in rustfmt) - #146094 (Make `Parser::parse_for_head` public for rustfmt usage) - #146102 (Remove dead code stemming from an old effects desugaring) r? `@ghost` `@rustbot` modify labels: rollup
|
@bors r- |
|
@oli-obk @clarfonthey @tgross35 I don't think this PR should have been rejected. Cow is often used to bridge cases where allocation may or may not occur. When it doesn't, we don't want to force non-const on the developer. With these const From traits, you can effectively use Cow in generic contents where the conditionality of the const depends on the implementor. For example: const trait Textify<'a> {
type TextType: [const] Into<Cow<'a, str>>;
fn text(&self) -> Self::TextType;
}In this case, the implementor can choose const or not based upon the implementation of |
|
Another simpler case is where conditional compilation is used. Here's a somewhat contrived example: #[cfg(feature = "alloc")]
type Text<'a> = alloc::borrow::Cow<'a, str>;
#[cfg(not(feature = "alloc"))]
type Text<'a> = &'a str;
const fn decode<'a>(bytes: &'a [u8]) -> Text<'a> {
core::str::from_utf8(bytes).unwrap().into() // Cow needs a const From impl here.
}The key point is that without the |
|
These implementations will absolutely wind up We do not have an official policy here (I've been meaning to write a proposal) so it's possible the others feel differently, but I would still prefer to wait until const traits are stable for implementations like this. There isn't any rush. See also #t-compiler/const-eval > SystemTime regression #146228 |
➜ rust git:(master) $ egrep -R '(const_convert|const_default)' library/alloc
library/alloc/src/lib.rs:#![feature(const_convert)]
library/alloc/src/lib.rs:#![feature(const_default)]
library/alloc/src/borrow.rs:#[rustc_const_unstable(feature = "const_convert", issue = "143773")]
library/alloc/src/borrow.rs:#[rustc_const_unstable(feature = "const_convert", issue = "143773")]
library/alloc/src/borrow.rs:#[rustc_const_unstable(feature = "const_convert", issue = "143773")]
library/alloc/src/vec/mod.rs:#[rustc_const_unstable(feature = "const_default", issue = "143894")]
library/alloc/src/string.rs:#[rustc_const_unstable(feature = "const_default", issue = "143894")]
library/alloc/src/collections/mod.rs:#[rustc_const_unstable(feature = "const_convert", issue = "143773")]
library/alloc/src/collections/mod.rs:#[rustc_const_unstable(feature = "const_convert", issue = "143773")]@tgross35 There are already const implementations in |
|
In retrospect I should have requested them dropped from #145279. I don't think there is any need to remove them at this time, given they aren't particularly invasive, but I don't think it should serve as a precedent for adding more. Again, others may feel differently and we don't yet have a policy. And the incremental change isn't much. But I really don't think anybody's const experimentation is blocked on wrappers around |
|
@tgross35 Mine is. That's why I wrote the patch. |
This is an expansion of #143773 for the
CowFromconversions.r? @oli-obk