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.

Sign up for GitHub

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

Jump to bottom

Add uDisplay for String and inline ufmt functions #460

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add uDisplay for String and inline ufmt functions #460

wants to merge 2 commits into from

Conversation

lmbollen
Copy link

Most likely the compiler already inlines the function calls, but afaik adding the annotation makes it more likely.

I could not build it locally due to:

   Compiling heapless v0.8.0 (/home/lucas/repos/heapless)
error[E0658]: use of unstable library feature 'build_hasher_simple_hash_one'
    --> src/indexmap.rs:1260:28
     |
1260 |     HashValue(build_hasher.hash_one(key) as u16)
     |                            ^^^^^^^^
     |
     = note: see issue #86161 <https://github.com/rust-lang/rust/issues/86161> for more information

For more information about this error, try `rustc --explain E0658`.
error: could not compile `heapless` due to previous error

But the implementation seems trivial and has been tested elsewhere.
I did not add uDisplay for Vec because the formatting is not trivial.

We might want to consider adding uDebug, but don't know what the rusty way to do so would be.

@lmbollen lmbollen mentioned this pull request Feb 28, 2024
Closed
@lmbollen lmbollen force-pushed the ufmt-udisplay-string branch from 572f4bd to fd446d8 Compare February 28, 2024 09:34
@reitermarkus
Copy link
Member

We might want to consider adding uDebug, but don't know what the rusty way to do so would be.

Does ufmt not already provide an implementation for str?

@reitermarkus
Copy link
Member

Apparently not yet: japaric/ufmt#52

@reitermarkus
Copy link
Member

inline(always) is almost never needed, so let's use only inline.

@Dirbaio
Copy link
Member

Dirbaio commented Jun 30, 2024

can you rebase and fix CI?

@lmbollen lmbollen force-pushed the ufmt-udisplay-string branch 4 times, most recently from 13827d7 to e29673f Compare July 1, 2024 07:43
@lmbollen
Copy link
Author

lmbollen commented Jul 1, 2024

can you rebase and fix CI?

Done, also changed #[inline(always)] to #[inline]

Dirbaio
Dirbaio reviewed Jul 1, 2024
View reviewed changes
Cargo.toml Outdated
@@ -42,11 +42,12 @@ mpmc_large = []
nightly = []

[dependencies]
portable-atomic = { version = "1.0", optional = true }
defmt = { version = ">=0.2.0,<0.4", optional = true }
Copy link
Member

Choose a reason for hiding this comment

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

please don't autoformat Cargo.toml

Dirbaio
Dirbaio reviewed Jul 1, 2024
View reviewed changes
CHANGELOG.md Outdated
@@ -74,6 +75,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- `ufmt-impl` is now `ufmt`
- `cas` is removed, atomic polyfilling is now opt-in via the `portable-atomic` feature.
- `Vec::as_mut_slice` is now a public method.
- `ufmt` functions are annotated with `inline(always)`.
Copy link
Member

Choose a reason for hiding this comment

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

this no longer applies

@lmbollen lmbollen force-pushed the ufmt-udisplay-string branch from e29673f to 1d80d72 Compare July 1, 2024 13:47
sosthene-nitrokey
sosthene-nitrokey suggested changes Jul 1, 2024
View reviewed changes
Copy link
Contributor

@sosthene-nitrokey sosthene-nitrokey left a comment

Choose a reason for hiding this comment

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

This should be generic over the StringInner and the storage used.

src/ufmt.rs Outdated
@@ -1,15 +1,28 @@
use crate::{storage::Storage, string::String, vec::VecInner};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
use crate::{storage::Storage, string::String, vec::VecInner};
use crate::{storage::Storage, string::StringInner, vec::VecInner};

src/ufmt.rs Outdated
use ufmt_write::uWrite;

impl<const SIZE: usize> uDisplay for String<SIZE> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
impl<const SIZE: usize> uDisplay for String<SIZE> {
impl<S: Storage> uDisplay for StringInner<S> {

lmbollen added 2 commits July 2, 2024 02:25
@lmbollen @Dirbaio
Add the uDisplay trait for String incl test.
c67a764 
Did not add `uDisplay` for `Vec` because there is no trivial implementation.
@lmbollen @Dirbaio
Add inline annotations to wrapping function calls in ufmt
c692a4b 
This should make the compiler inline the function calls to increase performance.
@Dirbaio Dirbaio force-pushed the ufmt-udisplay-string branch from 1d80d72 to c692a4b Compare July 2, 2024 00:26
@Dirbaio Dirbaio enabled auto-merge July 2, 2024 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dirbaio Dirbaio Dirbaio left review comments

@sosthene-nitrokey sosthene-nitrokey sosthene-nitrokey requested changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lmbollen @reitermarkus @Dirbaio @sosthene-nitrokey