Skip to content

Fix FormattingOptions instantiation with Default #135977

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

Merged
merged 1 commit into from
Jan 25, 2025

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Jan 24, 2025

The fill value by default should be set to ' ' (space), but the current implementation uses #[derive(Default)] which sets it to \0.

Note that FormattingOptions is being released as part of 1.85 (unstable) - so this might warrant a backport to that branch.

Tracking issue: #118117

Follow up from #118159

CC: @EliasHolzmann @programmerjake

r? @m-ou-se

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 24, 2025
@rust-log-analyzer

This comment has been minimized.

The `fill` value by default should be set to `' '` (space), but the current implementation uses `#[derive(Default)]` which sets it to `\0`
@programmerjake
Copy link
Member

Note that FormattingOptions is being released as part of 1.85 (unstable) - so this might warrant a backport to that branch.

afaik we generally don't backport changes that only affect unstable APIs, since they're intended to only work on nightly, not beta or stable. If this fixes some stable API then that's a different story.

@joboet
Copy link
Member

joboet commented Jan 24, 2025

FormattingOptions::default isn't yet used internally, so this didn't affect public behaviour and hence doesn't need backporting. Good catch, though!

@bors r+ rollup
r? joboet

@bors
Copy link
Collaborator

bors commented Jan 24, 2025

📌 Commit c9ae0bb has been approved by joboet

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 Jan 24, 2025
@bors bors merged commit 5c821ae into rust-lang:master Jan 25, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 25, 2025
@nyurik nyurik deleted the fix-fmt-options branch January 25, 2025 14:26
@EliasHolzmann
Copy link
Contributor

@nyurik Thanks, nice catch!

@nyurik
Copy link
Contributor Author

nyurik commented Feb 18, 2025

"There is (now) a lint for that" (tm)

rust-lang/rust-clippy#14234

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-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants