On Tue May 27, 2025 at 5:02 PM CEST, Tamir Duberstein wrote:
On Mon, May 26, 2025 at 7:01 PM Benno Lossin lossin@kernel.org wrote:
On Tue May 27, 2025 at 12:17 AM CEST, Tamir Duberstein wrote:
On Mon, May 26, 2025 at 10:48 AM Benno Lossin lossin@kernel.org wrote:
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
+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.
+/// 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.
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.
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?
- };
- 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