Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions utils/writeable/src/cmp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,31 @@ pub fn cmp_utf8(writeable: &impl Writeable, other: &[u8]) -> Ordering {
/// assert_eq!(Ordering::Less, writeable::cmp_str(&message, "Hello, Bob!"));
/// assert_eq!(Ordering::Less, (*message_str).cmp("Hello, Bob!"));
/// ```
///
/// This function can be combined with `writeable::concatenate!` to make an efficient
/// string-substring comparison:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how this gives you a string-substring comparison, the examples certainly don't show that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm? The example is comparing a string to a list of substrings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to me a string substring comparison is either eq("foo", 1, 2, "oo") or "foo".contains("oo").

the thing that concatenate! is doing is just concatenating Writeables without materializing them to Strings. I don't really see the main selling point as "string-substring comparison".

///
/// ```
/// use core::cmp::Ordering;
///
/// let en = "en";
/// let fr = "fr";
/// let au = "AU";
/// let us = "US";
///
/// assert_eq!(
/// Ordering::Less,
/// writeable::cmp_str(&writeable::concatenate!(en, '-', au), "en-US")
/// );
/// assert_eq!(
/// Ordering::Equal,
/// writeable::cmp_str(&writeable::concatenate!(en, '-', us), "en-US")
/// );
/// assert_eq!(
/// Ordering::Greater,
/// writeable::cmp_str(&writeable::concatenate!(fr, '-', us), "en-US")
/// );
/// ```
#[inline]
pub fn cmp_str(writeable: &impl Writeable, other: &str) -> Ordering {
cmp_utf8(writeable, other.as_bytes())
Expand Down
148 changes: 148 additions & 0 deletions utils/writeable/src/concat.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
// This file is part of ICU4X. For terms of use, please see the file
// called LICENSE at the top level of the ICU4X source tree
// (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ).

use crate::TryWriteable;
use crate::Writeable;
use core::fmt;

/// A [`Writeable`] that efficiently concatenates two other [`Writeable`]s.
///
/// See the [`concatenate!`] macro for a convenient way to make one of these.
///
/// # Examples
///
/// ```
/// use writeable::adapters::Concatenate;
/// use writeable::assert_writeable_eq;
///
/// assert_writeable_eq!(Concatenate("Number: ", 25), "Number: 25");
/// ```
///
/// With [`TryWriteable`]:
///
/// ```
/// use writeable::adapters::Concatenate;
/// use writeable::TryWriteable;
/// use writeable::assert_try_writeable_eq;
///
/// struct AlwaysPanic;
///
/// impl TryWriteable for AlwaysPanic {
/// type Error = &'static str;
/// fn try_write_to_parts<W: writeable::PartsWrite + ?Sized>(&self, _sink: &mut W) -> Result<Result<(), Self::Error>, core::fmt::Error> {
/// // Unreachable panic: the first Writeable errors,
/// // so the second Writeable is not evaluated.
/// panic!()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's weird to make the TryWriteable panic, because it has a non-panicking error mechanism. why not use that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is demonstrating that this code is unreachable for test; no one should write a TryWriteable like this.

But since it is in the docs of the writeable crate, I see your point that people may copy and paste this and think it is what they are supposed to do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe use unreachable! then!

/// }
/// }
///
/// let writeable0: Result<&str, &str> = Err("error message");
/// let writeable1 = AlwaysPanic;
///
/// assert_try_writeable_eq!(
/// Concatenate(writeable0, writeable1),
/// "error message",
/// Err("error message"),
/// )
/// ```
#[derive(Debug)]
#[allow(clippy::exhaustive_structs)] // designed for nesting
pub struct Concatenate<A, B>(pub A, pub B);

impl<A, B> Writeable for Concatenate<A, B>
where
A: Writeable,
B: Writeable,
{
#[inline]
fn write_to<W: fmt::Write + ?Sized>(&self, sink: &mut W) -> fmt::Result {
self.0.write_to(sink)?;
self.1.write_to(sink)
}
#[inline]
fn write_to_parts<S: crate::PartsWrite + ?Sized>(&self, sink: &mut S) -> fmt::Result {
self.0.write_to_parts(sink)?;
self.1.write_to_parts(sink)
}
#[inline]
fn writeable_length_hint(&self) -> crate::LengthHint {
self.0.writeable_length_hint() + self.1.writeable_length_hint()
}
}

impl<A, B, E> TryWriteable for Concatenate<A, B>
where
A: TryWriteable<Error = E>,
B: TryWriteable<Error = E>,
{
type Error = E;
#[inline]
fn try_write_to<W: fmt::Write + ?Sized>(
&self,
sink: &mut W,
) -> Result<Result<(), Self::Error>, fmt::Error> {
if let Err(e) = self.0.try_write_to(sink)? {
return Ok(Err(e));
}
self.1.try_write_to(sink)
}
#[inline]
fn try_write_to_parts<S: crate::PartsWrite + ?Sized>(
&self,
sink: &mut S,
) -> Result<Result<(), Self::Error>, fmt::Error> {
if let Err(e) = self.0.try_write_to_parts(sink)? {
return Ok(Err(e));
}
self.1.try_write_to_parts(sink)
}
#[inline]
fn writeable_length_hint(&self) -> crate::LengthHint {
self.0.writeable_length_hint() + self.1.writeable_length_hint()
}
}

impl<A, B> fmt::Display for Concatenate<A, B>
where
A: Writeable,
B: Writeable,
{
#[inline]
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
self.0.write_to(f)?;
self.1.write_to(f)
}
}

/// Returns a [`Writeable`] concatenating any number of [`Writeable`]s.
///
/// The macro resolves to a nested [`Concatenate`].
///
/// # Examples
///
/// ```
/// use writeable::assert_writeable_eq;
///
/// let concatenated = writeable::concatenate!(
/// "Health: ",
/// 5,
/// '/',
/// 8
/// );
///
/// assert_writeable_eq!(concatenated, "Health: 5/8");
/// ```
#[macro_export]
#[doc(hidden)] // macro
macro_rules! __concatenate {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concat breaks use writeable::* unfortunately because std::concat is in the default prelude. You can see the errors in an earlier commit on the branch:

https://github.com/unicode-org/icu4x/actions/runs/17571159926/job/49907497551

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Open to suggestions on other names.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

join?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concat_writeable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

writeable::concat_writeable! is redundant but maybe ok?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume it's mostly going to be imported

// Base case:
($x:expr) => ($x);
// `$x` followed by at least one `$y,`
($x:expr, $($y:expr),+) => (
// Call `concatenate!` recursively on the tail `$y`
$crate::adapters::Concatenate($x, $crate::concatenate!($($y),+))
)
}
#[doc(inline)]
pub use __concatenate as concatenate;
3 changes: 3 additions & 0 deletions utils/writeable/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
extern crate alloc;

mod cmp;
mod concat;
#[cfg(feature = "either")]
mod either;
mod impls;
Expand All @@ -93,13 +94,15 @@ use alloc::string::String;
use core::fmt;

pub use cmp::{cmp_str, cmp_utf8};
pub use concat::concatenate;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: do we think this should be toplevel or under adapters? I think either is fine, just calling out the choice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think writeable::concatenate! is nice, similar to writeable::cmp_str(), but not sure if we should worry about conflicts when people glob-import

pub use to_string_or_borrow::to_string_or_borrow;
pub use try_writeable::TryWriteable;

/// Helper types for trait impls.
pub mod adapters {
use super::*;

pub use concat::Concatenate;
pub use parts_write_adapter::CoreWriteAsPartsWrite;
pub use parts_write_adapter::WithPart;
pub use try_writeable::TryWriteableInfallibleAsWriteable;
Expand Down
Loading