Add writeable::adapters::Concatenate and writeable::concatenate! #6929

sffc · 2025-09-09T03:48:43Z

Alternative to #6921

gemini-code-assist · 2025-09-09T03:48:47Z

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

sffc · 2025-09-09T03:52:26Z

/gemini review

Copilot

Pull Request Overview

This PR adds a new Concat adapter and concatenate! macro to the writeable crate for efficiently concatenating multiple Writeable implementations. The changes provide a performant alternative to string concatenation by implementing the concatenation directly at the Writeable trait level.

Key changes:

Introduces Concat struct that implements Writeable and TryWriteable for concatenating two writeables
Adds concatenate! macro for convenient concatenation of multiple writeables
Updates documentation with examples showing how to use the new functionality

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
utils/writeable/src/lib.rs	Adds module declaration and public exports for the new concat functionality
utils/writeable/src/concat.rs	Implements the core `Concat` adapter struct and `concatenate!` macro with full trait implementations
utils/writeable/src/cmp.rs	Adds documentation example demonstrating usage of `concatenate!` macro with comparison functions

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

utils/writeable/src/concat.rs

Co-authored-by: Copilot <[email protected]>

gemini-code-assist

Code Review

This pull request introduces writeable::adapters::Concat and the writeable::concatenate! macro, providing an efficient way to concatenate multiple Writeable items. The implementation is solid and the new functionality is well-integrated. My review focuses on improving documentation clarity and addressing minor code style issues in the new concat.rs file and related documentation.

utils/writeable/src/cmp.rs

utils/writeable/src/concat.rs

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Manishearth

I like this

Manishearth · 2025-09-09T03:53:53Z

utils/writeable/src/lib.rs

 use core::fmt;

 pub use cmp::{cmp_str, cmp_utf8};
+pub use concat::concatenate;


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

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

utils/writeable/src/cmp.rs

robertbastian · 2025-09-09T07:47:17Z

utils/writeable/src/cmp.rs

+///
+/// assert_eq!(
+///     Ordering::Less,
+///     writeable::cmp_str(&writeable::concatenate!("en", '-', "AU"), "en-US")


this doesn't actually require concatenate (just write "en-AU"), can you use an example that does?

I prefer the example you have on the Concatenate type that combines impl Writeable for str with impl Writeable for i32. even with your update this does not really need concat.

robertbastian · 2025-09-09T07:50:37Z

utils/writeable/src/concat.rs

+/// ```
+#[macro_export]
+#[doc(hidden)] // macro
+macro_rules! __concatenate {


I think this should be concat: https://doc.rust-lang.org/stable/std/?search=concat

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

(Open to suggestions on other names.)

concat_writeable

writeable::concat_writeable! is redundant but maybe ok?

I assume it's mostly going to be imported

robertbastian · 2025-09-09T07:51:45Z

utils/writeable/src/cmp.rs

 /// ```
+///
+/// This function can be combined with `writeable::concatenate!` to make an efficient
+/// string-substring comparison:


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

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

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".

robertbastian

.

robertbastian · 2025-09-26T11:31:54Z

utils/writeable/src/concat.rs

+///     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!()


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

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.

maybe use unreachable! then!

CLAassistant · 2025-09-29T17:40:50Z

All committers have signed the CLA.

Add writeable::adapters::Concat and writeable::concat!

b9e074e

sffc requested a review from robertbastian as a code owner September 9, 2025 03:48

sffc requested a review from Manishearth September 9, 2025 03:48

sffc added 2 commits September 8, 2025 20:49

clippy

1634bbb

use concatenate to avoid conflicting the std concat

6b7b485

sffc requested a review from Copilot September 9, 2025 03:52

sffc changed the title ~~Add writeable::adapters::Concat and writeable::concat!~~ Add writeable::adapters::Concat and writeable::concatenate! Sep 9, 2025

Copilot AI reviewed Sep 9, 2025

View reviewed changes

utils/writeable/src/concat.rs Outdated Show resolved Hide resolved

utils/writeable/src/concat.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

1087867

Co-authored-by: Copilot <[email protected]>

gemini-code-assist bot reviewed Sep 9, 2025

View reviewed changes

utils/writeable/src/cmp.rs Outdated Show resolved Hide resolved

utils/writeable/src/concat.rs Outdated Show resolved Hide resolved

utils/writeable/src/concat.rs Show resolved Hide resolved

utils/writeable/src/concat.rs Show resolved Hide resolved

Apply suggestions from code review

5feaebb

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Manishearth previously approved these changes Sep 9, 2025

View reviewed changes

robertbastian reviewed Sep 9, 2025

View reviewed changes

sffc added 2 commits September 9, 2025 13:50

Feedback on test

274aad9

Concat -> Concatenate

26b4533

sffc dismissed Manishearth’s stale review via 26b4533 September 9, 2025 20:50

sffc requested a review from robertbastian September 9, 2025 20:51

sffc changed the title ~~Add writeable::adapters::Concat and writeable::concatenate!~~ Add writeable::adapters::Concatenate and writeable::concatenate! Sep 9, 2025

robertbastian reviewed Sep 10, 2025

View reviewed changes

robertbastian reviewed Sep 26, 2025

View reviewed changes

robertbastian added the waiting-on-author PRs waiting for action from the author for >7 days label Sep 26, 2025

Add writeable::adapters::Concatenate and writeable::concatenate! #6929

Are you sure you want to change the base?

Add writeable::adapters::Concatenate and writeable::concatenate! #6929

Uh oh!

Conversation

sffc commented Sep 9, 2025

Uh oh!

gemini-code-assist bot commented Sep 9, 2025

Uh oh!

sffc commented Sep 9, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertbastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CLAassistant commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

CLAassistant commented Sep 29, 2025 •

edited

Loading