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

Tracking Issue for FormattingOptions #118117

Open
2 of 6 tasks
EliasHolzmann opened this issue Nov 21, 2023 · 3 comments
Open
2 of 6 tasks

Tracking Issue for FormattingOptions #118117

EliasHolzmann opened this issue Nov 21, 2023 · 3 comments
Assignees
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@EliasHolzmann
Copy link
Contributor

EliasHolzmann commented Nov 21, 2023

Feature gate: #![feature(formatting_options)]

This is a tracking issue for fmt::FormattingOptions.

FormattingOptions can be used to construct custom Formatters at runtime.

Public API

#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub struct FormattingOptions {}

enum Sign {
    Plus, 
    Minus
}

impl FormattingOptions {
    pub fn new() -> Self;

    pub fn sign(&mut self, sign: Option<Sign>) -> &mut Self;
    pub fn zero_pad(&mut self, zero_pad: bool) -> &mut Self;
    pub fn alternate(&mut self, alternate: bool) -> &mut Self;
    pub fn fill(&mut self, fill: Option<char>) -> &mut Self;
    pub fn alignment(&mut self, alignment: Option<Alignment>) -> &mut Self;
    pub fn width(&mut self, width: Option<usize>) -> &mut Self;
    pub fn precision(&mut self, precision: Option<usize>) -> &mut Self;

    pub fn get_sign(&self) -> Option<Sign>;
    pub fn get_zero_pad(&self) -> bool;
    pub fn get_alternate(&self) -> bool;
    pub fn get_fill(&self) -> Option<char>;
    pub fn get_alignment(&self) -> Option<Alignment>;
    pub fn get_width(&self) -> Option<usize>;
    pub fn get_precision(&self) -> Option<usize>;
    
    pub fn create_formatter<'a>(self, write: &'a mut (dyn Write + 'a)) -> Formatter<'a>;
}

impl<'a> Formatter<'a> {
    pub fn new(write: &'a mut (dyn Write + 'a), options: FormattingOptions) -> Self;
    pub fn with_options(&mut self, options: FormattingOptions) -> Self;
    
    pub fn sign(&self) -> Option<Sign>;

    pub fn options(&self) -> FormattingOptions;
}

Steps / History

Unresolved Questions

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/api-change-proposals.html

  2. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html

@EliasHolzmann EliasHolzmann added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 21, 2023
@EliasHolzmann
Copy link
Contributor Author

@rustbot claim

@dtolnay dtolnay changed the title Tracking Issue for XXX Tracking Issue for FormattingOptions Nov 21, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 15, 2024
…<try>

Implementation of `fmt::FormatttingOptions`

Tracking issue: rust-lang#118117

Public API:
```rust
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub struct FormattingOptions { … }
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub enum Sign {
    Plus,
    Minus
}
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub enum DebugAsHex {
    Lower,
    Upper
}

impl FormattingOptions {
    pub fn new() -> Self;
    pub fn sign(&mut self, sign: Option<Sign>) -> &mut Self;
    pub fn sign_aware_zero_pad(&mut self, sign_aware_zero_pad: bool) -> &mut Self;
    pub fn alternate(&mut self, alternate: bool) -> &mut Self;
    pub fn fill(&mut self, fill: char) -> &mut Self;
    pub fn align(&mut self, alignment: Option<Alignment>) -> &mut Self;
    pub fn width(&mut self, width: Option<usize>) -> &mut Self;
    pub fn precision(&mut self, precision: Option<usize>) -> &mut Self;
    pub fn debug_as_hex(&mut self, debug_as_hex: Option<DebugAsHex>) -> &mut Self;

    pub fn get_sign(&self) -> Option<Sign>;
    pub fn get_sign_aware_zero_pad(&self) -> bool;
    pub fn get_alternate(&self) -> bool;
    pub fn get_fill(&self) -> char;
    pub fn get_align(&self) -> Option<Alignment>;
    pub fn get_width(&self) -> Option<usize>;
    pub fn get_precision(&self) -> Option<usize>;
    pub fn get_debug_as_hex(&self) -> Option<DebugAsHex>;

    pub fn create_formatter<'a>(self, write: &'a mut (dyn Write + 'a)) -> Formatter<'a>;
    }

impl<'a> Formatter<'a> {
    pub fn new(write: &'a mut (dyn Write + 'a), options: FormattingOptions) -> Self;
    pub fn with_options<'b>(&'b mut self, options: FormattingOptions) -> Formatter<'b>;
    pub fn sign(&self) -> Option<Sign>;

    pub fn options(&self) -> FormattingOptions;
}
```

Relevant changes from the public API in the tracking issue (I'm leaving out some stuff I consider obvious mistakes, like missing `#[derive(..)]`s and `pub` specifiers):

- `enum DebugAsHex`/`FormattingOptions::debug_as_hex`/`FormattingOptions::get_debug_as_hex`: To support `{:x?}` as well as `{:X?}`. I had completely missed these options in the ACP. I'm open for any and all bikeshedding, not married to the name.
- `fill`/`get_fill` now takes/returns `char` instead of `Option<char>`. This simply mirrors what `Formatter::fill` returns (with default being `' '`).
- Changed `zero_pad`/`get_zero_pad` to `sign_aware_zero_pad`/`get_sign_aware_zero_pad`. This also mirrors `Formatter::sign_aware_zero_pad`. While I'm not a fan of this quite verbose name, I do believe that having the interface of `Formatter` and `FormattingOptions` be compatible is more important.
- For the same reason, renamed `alignment`/`get_alignment` to `aling`/`get_align`.
- Deviating from my initial idea, `Formatter::with_options` returns a `Formatter` which has the lifetime of the `self` reference as its generic lifetime parameter (in the original API spec, the generic lifetime of the returned `Formatter` was the generic lifetime used by `self` instead). Otherwise, one could construct two `Formatter`s that both mutably borrow the same underlying buffer, which would be unsound. This solution still has performance benefits over simply using `Formatter::new`, so I believe it is worthwhile to keep this method.
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 16, 2024
…<try>

Implementation of `fmt::FormattingOptions`

Tracking issue: rust-lang#118117

Public API:
```rust
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub struct FormattingOptions { … }
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub enum Sign {
    Plus,
    Minus
}
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub enum DebugAsHex {
    Lower,
    Upper
}

impl FormattingOptions {
    pub fn new() -> Self;
    pub fn sign(&mut self, sign: Option<Sign>) -> &mut Self;
    pub fn sign_aware_zero_pad(&mut self, sign_aware_zero_pad: bool) -> &mut Self;
    pub fn alternate(&mut self, alternate: bool) -> &mut Self;
    pub fn fill(&mut self, fill: char) -> &mut Self;
    pub fn align(&mut self, alignment: Option<Alignment>) -> &mut Self;
    pub fn width(&mut self, width: Option<usize>) -> &mut Self;
    pub fn precision(&mut self, precision: Option<usize>) -> &mut Self;
    pub fn debug_as_hex(&mut self, debug_as_hex: Option<DebugAsHex>) -> &mut Self;

    pub fn get_sign(&self) -> Option<Sign>;
    pub fn get_sign_aware_zero_pad(&self) -> bool;
    pub fn get_alternate(&self) -> bool;
    pub fn get_fill(&self) -> char;
    pub fn get_align(&self) -> Option<Alignment>;
    pub fn get_width(&self) -> Option<usize>;
    pub fn get_precision(&self) -> Option<usize>;
    pub fn get_debug_as_hex(&self) -> Option<DebugAsHex>;

    pub fn create_formatter<'a>(self, write: &'a mut (dyn Write + 'a)) -> Formatter<'a>;
    }

impl<'a> Formatter<'a> {
    pub fn new(write: &'a mut (dyn Write + 'a), options: FormattingOptions) -> Self;
    pub fn with_options<'b>(&'b mut self, options: FormattingOptions) -> Formatter<'b>;
    pub fn sign(&self) -> Option<Sign>;

    pub fn options(&self) -> FormattingOptions;
}
```

Relevant changes from the public API in the tracking issue (I'm leaving out some stuff I consider obvious mistakes, like missing `#[derive(..)]`s and `pub` specifiers):

- `enum DebugAsHex`/`FormattingOptions::debug_as_hex`/`FormattingOptions::get_debug_as_hex`: To support `{:x?}` as well as `{:X?}`. I had completely missed these options in the ACP. I'm open for any and all bikeshedding, not married to the name.
- `fill`/`get_fill` now takes/returns `char` instead of `Option<char>`. This simply mirrors what `Formatter::fill` returns (with default being `' '`).
- Changed `zero_pad`/`get_zero_pad` to `sign_aware_zero_pad`/`get_sign_aware_zero_pad`. This also mirrors `Formatter::sign_aware_zero_pad`. While I'm not a fan of this quite verbose name, I do believe that having the interface of `Formatter` and `FormattingOptions` be compatible is more important.
- For the same reason, renamed `alignment`/`get_alignment` to `aling`/`get_align`.
- Deviating from my initial idea, `Formatter::with_options` returns a `Formatter` which has the lifetime of the `self` reference as its generic lifetime parameter (in the original API spec, the generic lifetime of the returned `Formatter` was the generic lifetime used by `self` instead). Otherwise, one could construct two `Formatter`s that both mutably borrow the same underlying buffer, which would be unsound. This solution still has performance benefits over simply using `Formatter::new`, so I believe it is worthwhile to keep this method.
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 5, 2024
…<try>

Implementation of `fmt::FormattingOptions`

Tracking issue: rust-lang#118117

Public API:
```rust
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub struct FormattingOptions { … }
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub enum Sign {
    Plus,
    Minus
}
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub enum DebugAsHex {
    Lower,
    Upper
}

impl FormattingOptions {
    pub fn new() -> Self;
    pub fn sign(&mut self, sign: Option<Sign>) -> &mut Self;
    pub fn sign_aware_zero_pad(&mut self, sign_aware_zero_pad: bool) -> &mut Self;
    pub fn alternate(&mut self, alternate: bool) -> &mut Self;
    pub fn fill(&mut self, fill: char) -> &mut Self;
    pub fn align(&mut self, alignment: Option<Alignment>) -> &mut Self;
    pub fn width(&mut self, width: Option<usize>) -> &mut Self;
    pub fn precision(&mut self, precision: Option<usize>) -> &mut Self;
    pub fn debug_as_hex(&mut self, debug_as_hex: Option<DebugAsHex>) -> &mut Self;

    pub fn get_sign(&self) -> Option<Sign>;
    pub fn get_sign_aware_zero_pad(&self) -> bool;
    pub fn get_alternate(&self) -> bool;
    pub fn get_fill(&self) -> char;
    pub fn get_align(&self) -> Option<Alignment>;
    pub fn get_width(&self) -> Option<usize>;
    pub fn get_precision(&self) -> Option<usize>;
    pub fn get_debug_as_hex(&self) -> Option<DebugAsHex>;

    pub fn create_formatter<'a>(self, write: &'a mut (dyn Write + 'a)) -> Formatter<'a>;
    }

impl<'a> Formatter<'a> {
    pub fn new(write: &'a mut (dyn Write + 'a), options: FormattingOptions) -> Self;
    pub fn with_options<'b>(&'b mut self, options: FormattingOptions) -> Formatter<'b>;
    pub fn sign(&self) -> Option<Sign>;

    pub fn options(&self) -> FormattingOptions;
}
```

Relevant changes from the public API in the tracking issue (I'm leaving out some stuff I consider obvious mistakes, like missing `#[derive(..)]`s and `pub` specifiers):

- `enum DebugAsHex`/`FormattingOptions::debug_as_hex`/`FormattingOptions::get_debug_as_hex`: To support `{:x?}` as well as `{:X?}`. I had completely missed these options in the ACP. I'm open for any and all bikeshedding, not married to the name.
- `fill`/`get_fill` now takes/returns `char` instead of `Option<char>`. This simply mirrors what `Formatter::fill` returns (with default being `' '`).
- Changed `zero_pad`/`get_zero_pad` to `sign_aware_zero_pad`/`get_sign_aware_zero_pad`. This also mirrors `Formatter::sign_aware_zero_pad`. While I'm not a fan of this quite verbose name, I do believe that having the interface of `Formatter` and `FormattingOptions` be compatible is more important.
- For the same reason, renamed `alignment`/`get_alignment` to `aling`/`get_align`.
- Deviating from my initial idea, `Formatter::with_options` returns a `Formatter` which has the lifetime of the `self` reference as its generic lifetime parameter (in the original API spec, the generic lifetime of the returned `Formatter` was the generic lifetime used by `self` instead). Otherwise, one could construct two `Formatter`s that both mutably borrow the same underlying buffer, which would be unsound. This solution still has performance benefits over simply using `Formatter::new`, so I believe it is worthwhile to keep this method.
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 6, 2024
…m-ou-se

Implementation of `fmt::FormattingOptions`

Tracking issue: rust-lang#118117

Public API:
```rust
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub struct FormattingOptions { … }
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub enum Sign {
    Plus,
    Minus
}
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub enum DebugAsHex {
    Lower,
    Upper
}

impl FormattingOptions {
    pub fn new() -> Self;
    pub fn sign(&mut self, sign: Option<Sign>) -> &mut Self;
    pub fn sign_aware_zero_pad(&mut self, sign_aware_zero_pad: bool) -> &mut Self;
    pub fn alternate(&mut self, alternate: bool) -> &mut Self;
    pub fn fill(&mut self, fill: char) -> &mut Self;
    pub fn align(&mut self, alignment: Option<Alignment>) -> &mut Self;
    pub fn width(&mut self, width: Option<usize>) -> &mut Self;
    pub fn precision(&mut self, precision: Option<usize>) -> &mut Self;
    pub fn debug_as_hex(&mut self, debug_as_hex: Option<DebugAsHex>) -> &mut Self;

    pub fn get_sign(&self) -> Option<Sign>;
    pub fn get_sign_aware_zero_pad(&self) -> bool;
    pub fn get_alternate(&self) -> bool;
    pub fn get_fill(&self) -> char;
    pub fn get_align(&self) -> Option<Alignment>;
    pub fn get_width(&self) -> Option<usize>;
    pub fn get_precision(&self) -> Option<usize>;
    pub fn get_debug_as_hex(&self) -> Option<DebugAsHex>;

    pub fn create_formatter<'a>(self, write: &'a mut (dyn Write + 'a)) -> Formatter<'a>;
    }

impl<'a> Formatter<'a> {
    pub fn new(write: &'a mut (dyn Write + 'a), options: FormattingOptions) -> Self;
    pub fn with_options<'b>(&'b mut self, options: FormattingOptions) -> Formatter<'b>;
    pub fn sign(&self) -> Option<Sign>;

    pub fn options(&self) -> FormattingOptions;
}
```

Relevant changes from the public API in the tracking issue (I'm leaving out some stuff I consider obvious mistakes, like missing `#[derive(..)]`s and `pub` specifiers):

- `enum DebugAsHex`/`FormattingOptions::debug_as_hex`/`FormattingOptions::get_debug_as_hex`: To support `{:x?}` as well as `{:X?}`. I had completely missed these options in the ACP. I'm open for any and all bikeshedding, not married to the name.
- `fill`/`get_fill` now takes/returns `char` instead of `Option<char>`. This simply mirrors what `Formatter::fill` returns (with default being `' '`).
- Changed `zero_pad`/`get_zero_pad` to `sign_aware_zero_pad`/`get_sign_aware_zero_pad`. This also mirrors `Formatter::sign_aware_zero_pad`. While I'm not a fan of this quite verbose name, I do believe that having the interface of `Formatter` and `FormattingOptions` be compatible is more important.
- For the same reason, renamed `alignment`/`get_alignment` to `aling`/`get_align`.
- Deviating from my initial idea, `Formatter::with_options` returns a `Formatter` which has the lifetime of the `self` reference as its generic lifetime parameter (in the original API spec, the generic lifetime of the returned `Formatter` was the generic lifetime used by `self` instead). Otherwise, one could construct two `Formatter`s that both mutably borrow the same underlying buffer, which would be unsound. This solution still has performance benefits over simply using `Formatter::new`, so I believe it is worthwhile to keep this method.
@joboet
Copy link
Member

joboet commented Jan 22, 2025

I was wondering: could the signature for Formatter::new be changed to take a reference to the FormattingOptions instead of passing them by value? That way, LLVM wouldn't need to copy around the options as often, which seems like it could improve performance and binary size in cases where the same options are used multiple times, for instance when using the default options.

joboet added a commit to joboet/rust that referenced this issue Jan 24, 2025
Fix `FormattingOptions` instantiation with `Default`

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: rust-lang#118117

Follow up from rust-lang#118159

CC: `@EliasHolzmann` `@programmerjake`

r? `@m-ou-se`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 25, 2025
Fix `FormattingOptions` instantiation with `Default`

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: rust-lang#118117

Follow up from rust-lang#118159

CC: ``@EliasHolzmann`` ``@programmerjake``

r? ``@m-ou-se``
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 25, 2025
Rollup merge of rust-lang#135977 - nyurik:fix-fmt-options, r=joboet

Fix `FormattingOptions` instantiation with `Default`

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: rust-lang#118117

Follow up from rust-lang#118159

CC: ``@EliasHolzmann`` ``@programmerjake``

r? ``@m-ou-se``
@EliasHolzmann
Copy link
Contributor Author

@joboet I don't think so. Formatter directly holds a FormattingOptions internally: https://doc.rust-lang.org/nightly/src/core/fmt/mod.rs.html#524

If Formatter::new takes its FormattingOptions by reference instead of by value, we'd have to copy the options in that function anyway. (Instead, you probably could have the contents of FormattingOptions behind a Rc so that copying/cloning is cheaper, which might indeed improve performance for use cases with heavy FormattingOptions reuse. However, I don't think that this will compensate the ref-counting overhead that is in most use cases probably unnecessary.)

In most cases, when the FormattingOptions can be used multiple times, the target buffer of the resulting Formatter is probably also the same – in this case, you can just use the Formatter you construct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

2 participants