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

Change Format::format to take Formatter argument by value #305

Merged
merged 9 commits into from
Dec 16, 2020

Conversation

BriocheBerlin
Copy link
Contributor

Closes #277

justahero and others added 4 commits December 15, 2020 16:48
This changes the method signature of the `Format::format` trait function
to take the `Formatter` instance over the previous `&mut Formatter`
argument.

The previous `Formatter` is renamed to `InternalFormatter`, a new type `Formatter` holds a single `inner` field to keep a mutable reference to the `InternalFormatter`.

* update all implementations of `format`

Co-authored-by: japaric <jorge.aparicio@ferrous-systems.com>
Co-authored-by: BriocheBerlin <brigitte.markmann@asquera.de>
* change `#[derive(Format)]` to use `defmt::Formatter` instead of
  mutable ref
* delegate calls to InternalFormatter in macro
* change type in global logger to use InternalFormatter instead of
  Formatter
* fix compilation of defmt in test mode

**Note** there are still some compilation errors

Co-authored-by: japaric <jorge.aparicio@ferrous-systems.com>
Co-authored-by: BriocheBerlin <brigitte.markmann@asquera.de>
Fixes impl formatter; moves from_raw from Formatter to InternalFormatter,
depending on use of unstable-test feature.
Fixes compile errors in test/encode.rs file.
@japaric
Copy link
Member

japaric commented Dec 15, 2020

https://github.com/knurling-rs/defmt/pull/305/checks?check_run_id=1558757983#step:6:117

  --> firmware/qemu/src/bin/double-write.rs:16:9
   |
15 |         defmt::write!(fmt, "one");
   |         -------------------------- value moved here
16 |         defmt::write!(fmt, "two");
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^ value used here after move
   |
   = note: move occurs because `fmt.inner` has type `&mut InternalFormatter`, which does not implement the `Copy` trait
   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

this test (double-write.rs) can be removed. With these changes we don't need the runtime check because the issue is prevent at compile time.

@japaric
Copy link
Member

japaric commented Dec 15, 2020

Some follow-up work to do here. It doesn't need to be done in this PR but I wanted to jot down some notes somewhere:

  • remove double/nested write! runtime check (no longer needed because we prevent the issue at compile time)
  • update book / docs to mention the limitations of write! and the new Format::format signature
  • add compile time tests that check that one cannot invoke write! twice within a Format::format implementation

@japaric
Copy link
Member

japaric commented Dec 15, 2020

https://github.com/knurling-rs/defmt/pull/305/checks?check_run_id=1558757817#step:6:147

a code snippet needs to be updated in the book

``` rust
# extern crate defmt;
# struct X;
impl defmt::Format for X {
fn format(&self, f: &mut defmt::Formatter) {
// ^ this is a handle to the global logger
defmt::info!("Hello!");
// ..
}
}
```

like with API documentation, we test that the snippets compile to keep them in sync with library changes

@japaric
Copy link
Member

japaric commented Dec 16, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 16, 2020

Build succeeded:

@bors bors bot merged commit 2ffdf3c into main Dec 16, 2020
@bors bors bot deleted the formatter-by-value branch December 16, 2020 10:09
japaric added a commit that referenced this pull request Dec 16, 2020
when this feature was added, it was possible to call `write!` more than once from a `Format::format`
implementation. that operation corrupted the data stream so a runtime check was added to `write!` to
prevent the data corruption by panicking on a double use of `write!`

PR #305 makes `write!` consume the `Formatter` value so the above issue is now prevented at compile
time. The runtime check is no longer necessary (failure case is unreachable) so we remove it
bors bot added a commit that referenced this pull request Dec 16, 2020
310: write! macro: remove runtime check r=jonas-schievink a=japaric

when this feature was added, it was possible to call `write!` more than once from a `Format::format`
implementation. that operation corrupted the data stream so a runtime check was added to `write!` to
prevent the data corruption by panicking on a double use of `write!`

PR #305 makes `write!` consume the `Formatter` value so the above issue is now prevented at compile
time. The runtime check is no longer necessary (failure case is unreachable) so we remove it

Co-authored-by: Jorge Aparicio <jorge.aparicio@ferrous-systems.com>
bors bot added a commit that referenced this pull request Dec 17, 2020
311: remove unused snapshot file r=jonas-schievink a=japaric

the `double-write` test was removed in PR #305 

Co-authored-by: Jorge Aparicio <jorge.aparicio@ferrous-systems.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

consider making Format::format take the Formatter by value
3 participants