On Tue May 27, 2025 at 5:02 PM CEST, Tamir Duberstein wrote:

On Mon, May 26, 2025 at 7:01 PM Benno Lossin [off-list ref] wrote:

quoted

On Tue May 27, 2025 at 12:17 AM CEST, Tamir Duberstein wrote:

quoted

On Mon, May 26, 2025 at 10:48 AM Benno Lossin [off-list ref] wrote:

quoted

On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:

quoted

+impl_display_forward!(
+    bool,
+    char,
+    core::panic::PanicInfo<'_>,
+    crate::str::BStr,
+    fmt::Arguments<'_>,
+    i128,
+    i16,
+    i32,
+    i64,
+    i8,
+    isize,
+    str,
+    u128,
+    u16,
+    u32,
+    u64,
+    u8,
+    usize,
+    {<T: ?Sized>} crate::sync::Arc<T> {where crate::sync::Arc<T>: fmt::Display},
+    {<T: ?Sized>} crate::sync::UniqueArc<T> {where crate::sync::UniqueArc<T>: fmt::Display},
+);

If we use `{}` instead of `()`, then we can format the contents
differently:

    impl_display_forward! {
        i8, i16, i32, i64, i128, isize,
        u8, u16, u32, u64, u128, usize,
        bool, char, str,
        crate::str::BStr,
        fmt::Arguments<'_>,
        core::panic::PanicInfo<'_>,
        {<T: ?Sized>} crate::sync::Arc<T> {where Self: fmt::Display},
        {<T: ?Sized>} crate::sync::UniqueArc<T> {where Self: fmt::Display},
    }

Is that formatting better? rustfmt refuses to touch it either way.

Yeah rustfmt doesn't touch macro parameters enclosed in `{}`. I think
it's better.

OK, but why? This seems entirely subjective.

If more types are added to the list, it will grow over one screen size.
With my formatting, leaving related types on a single line, that will
only happen much later.

quoted

quoted

quoted

quoted

+/// Please see [`crate::fmt`] for documentation.
+pub(crate) fn fmt(input: TokenStream) -> TokenStream {
+    let mut input = input.into_iter();
+
+    let first_opt = input.next();
+    let first_owned_str;
+    let mut names = BTreeSet::new();
+    let first_lit = {
+        let Some((mut first_str, first_lit)) = (match first_opt.as_ref() {
+            Some(TokenTree::Literal(first_lit)) => {
+                first_owned_str = first_lit.to_string();
+                Some(first_owned_str.as_str()).and_then(|first| {
+                    let first = first.strip_prefix('"')?;
+                    let first = first.strip_suffix('"')?;
+                    Some((first, first_lit))
+                })
+            }
+            _ => None,
+        }) else {
+            return first_opt.into_iter().chain(input).collect();
+        };

This usage of let-else + match is pretty confusing and could just be a
single match statement.

I don't think so. Can you try rewriting it into the form you like?

    let (mut first_str, first_lit) match first_opt.as_ref() {
        Some(TokenTree::Literal(lit)) if lit.to_string().starts_with('"') => {
            let contents = lit.to_string();
            let contents = contents.strip_prefix('"').unwrap().strip_suffix('"').unwrap();
            ((contents, lit))
        }
        _ => return first_opt.into_iter().chain(input).collect(),
    };

What happens if the invocation is utterly malformed, e.g.
`fmt!("hello)`? You're unwrapping here, which I intentionally avoid.

That example won't even survive lexing (macros always will get valid
rust tokens as input). If a literal begins with a `"`, it also will end
with one AFAIK.

quoted

Yes it will error like that, but if we do the replacement only when the
syntax is correct, there also will be compile errors because of a
missing `Display` impl, or is that not the case?

I'm not sure - I would guess syntax errors "mask" typeck errors.

I checked and it seems to be so, that's good.

quoted

quoted

quoted

quoted

+                    first_str = rest;
+                    continue;
+                }
+                let name = name.split_once(':').map_or(name, |(name, _)| name);
+                if !name.is_empty() && !name.chars().all(|c| c.is_ascii_digit()) {
+                    names.insert(name);
+                }
+                break;
+            }
+        }
+        first_lit

`first_lit` is not modified, so could we just the code above it into a
block instead of keeping it in the expr for `first_lit`?

As above, can you suggest the alternate form you like better? The
gymnastics here are all in service of being able to let malformed
input fall through to core::format_args which will do the hard work of
producing good diagnostics.

I don't see how this is hard, just do:

    let (first_str, first_lit) = ...;

It requires you to unwrap, like you did above, which is what I'm
trying to avoid.

How so? What do you need to unwrap?

quoted

quoted

quoted

quoted

+    };
+
+    let first_span = first_lit.span();
+    let adapt = |expr| {
+        let mut borrow =
+            TokenStream::from_iter([TokenTree::Punct(Punct::new('&', Spacing::Alone))]);
+        borrow.extend(expr);
+        make_ident(first_span, ["kernel", "fmt", "Adapter"])
+            .chain([TokenTree::Group(Group::new(Delimiter::Parenthesis, borrow))])

This should be fine with using `quote!`:

    quote!(::kernel::fmt::Adapter(&#expr))

Yeah, I have a local commit that uses quote_spanned to remove all the
manual constructions.

I don't think that you need `quote_spanned` here at all. If you do, then
let me know, something weird with spans is going on then.

You need to give idents a span, so each of `kernel`, `fmt`, and
`adapter` need a span. I *could* use `quote!` and get whatever span it
uses (mixed_site) but I'd rather retain control.

Please use `quote!` if it works. No need to make this more complex than
it already is. If it doesn't work then that's another story.

---
Cheers,
Benno