Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor serialize/span tweaks #125391

Merged
merged 3 commits into from
May 22, 2024
Merged

Conversation

nnethercote
Copy link
Contributor

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 22, 2024
@@ -120,7 +120,7 @@ fn decode_field(field: &syn::Field) -> proc_macro2::TokenStream {
let __decoder = quote! { __decoder };
// Use the span of the field for the method call, so
// that backtraces will point to the field.
quote_spanned! {field_span=> #decode_inner_method(#__decoder) }
quote_spanned! { field_span => #decode_inner_method(#__decoder) }
Copy link
Member

Choose a reason for hiding this comment

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

This isn't concordant with the way that the macro suggests it should be formatted: https://docs.rs/proc-quote/latest/proc_quote/macro.quote_spanned.html#syntax

The lack of space before the => should look jarring to Rust programmers and this is intentional. The formatting is designed to be visibly off-balance and draw the eye a particular way, due to the span expression being evaluated in the context of the procedural macro and the remaining tokens being evaluated in the generated code.

Copy link
Member

Choose a reason for hiding this comment

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

TIL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lolwut?

I've removed the space. After that, I count five calls to quote_unspanned in the repo that omit the space before the => as advised and five that don't. The docs also say you should use parens when the quote_unspanned invocation fits on one line, and braces when it doesn't -- one of the occurrences in the repo follows that advice, and nine don't. Seems like making unusual formatting suggestions in docs doesn't work that well?

For something that wasn't obvious to me.
Because explicit macro imports are better than implicit macro imports.
@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 22, 2024

📌 Commit e60c191 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2024
fmease added a commit to fmease/rust that referenced this pull request May 22, 2024
…=compiler-errors

Minor serialize/span tweaks

r? `@jackh726`
bors added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#125043 (reference type safety invariant docs: clarification)
 - rust-lang#125306 (Force the inner coroutine of an async closure to `move` if the outer closure is `move` and `FnOnce`)
 - rust-lang#125355 (Use Backtrace::force_capture instead of Backtrace::capture in rustc_log)
 - rust-lang#125378 (remove tracing tree indent lines)
 - rust-lang#125391 (Minor serialize/span tweaks)
 - rust-lang#125395 (Remove unnecessary `.md` from the documentation sidebar)
 - rust-lang#125399 (Stop using `to_hir_binop` in codegen)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#125043 (reference type safety invariant docs: clarification)
 - rust-lang#125306 (Force the inner coroutine of an async closure to `move` if the outer closure is `move` and `FnOnce`)
 - rust-lang#125355 (Use Backtrace::force_capture instead of Backtrace::capture in rustc_log)
 - rust-lang#125382 (rustdoc: rename `issue-\d+.rs` tests to have meaningful names (part 7))
 - rust-lang#125391 (Minor serialize/span tweaks)
 - rust-lang#125395 (Remove unnecessary `.md` from the documentation sidebar)
 - rust-lang#125399 (Stop using `to_hir_binop` in codegen)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#125043 (reference type safety invariant docs: clarification)
 - rust-lang#125306 (Force the inner coroutine of an async closure to `move` if the outer closure is `move` and `FnOnce`)
 - rust-lang#125355 (Use Backtrace::force_capture instead of Backtrace::capture in rustc_log)
 - rust-lang#125382 (rustdoc: rename `issue-\d+.rs` tests to have meaningful names (part 7))
 - rust-lang#125391 (Minor serialize/span tweaks)
 - rust-lang#125395 (Remove unnecessary `.md` from the documentation sidebar)
 - rust-lang#125399 (Stop using `to_hir_binop` in codegen)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0646c7d into rust-lang:master May 22, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 22, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2024
Rollup merge of rust-lang#125391 - nnethercote:serialize-rs-tweaks, r=compiler-errors

Minor serialize/span tweaks

r? ``@jackh726``
@nnethercote nnethercote deleted the serialize-rs-tweaks branch May 23, 2024 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants