Skip to content

Char improvements#242

Open
Kixunil wants to merge 5 commits intorust-bitcoin:masterfrom
Kixunil:char-improvements
Open

Char improvements#242
Kixunil wants to merge 5 commits intorust-bitcoin:masterfrom
Kixunil:char-improvements

Conversation

@Kixunil
Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil commented Apr 21, 2026

These are a few follow ups to #236

I'm too tired for tests rn, will add them later.

The comment contained the typical mistake os saying "safe" when "sound"
is clearly meant (transmutes are never safe but may be sound), so this
fixes it.
@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Apr 21, 2026

utACK a3b6640.

Arguably the "Because" lines in the doccomments are unnecessary. From a user point of view, these conversions make sense because we've provided a safe method that provides them. The user doesn't care what representation magic is needed to get it to work.

Tests would be nice, I guess. The code appears to be correct, though it's much more aggressively optimized than anybody else would do.

CI is failing because of a clippy lint (looks like one of your pointer casts has a convenience function now) and also the API files, which if they annoy you too much, me or Tobin can add.

Kixunil added 3 commits April 22, 2026 15:09
Originally it was proposed that `AsRef<str> for [Char]` and other
conversions are implemented. However these are disallowed by the orphan
rules. The conversions are still desirable, so this commit adds them as
"static" methods of `Char`.

This specifically adds only conversions to slices since these are most
common and arrays will coerce to them if needed.
`Char` should be very similar to `char` but it was missing `Display`
impl. This commit adds it by casting `&char` to `&str` and calling that
which gives us formatting options for free.
Naively derived `Debug` impl on `Char` would produce the character names
rather than the characters themselves like `char` does. To improve
consistency and brevity, this manually implements `Debug` by creating a
stack-allocated `'{x}'` string and formatting that one.
@Kixunil Kixunil force-pushed the char-improvements branch from a3b6640 to 2713e69 Compare April 22, 2026 13:38
@Kixunil
Copy link
Copy Markdown
Collaborator Author

Kixunil commented Apr 22, 2026

Addressed all the feedback, including adding tests.

@apoelstra
Copy link
Copy Markdown
Member

Can you run the formatter?

Comment thread src/lib.rs
Comment on lines +296 to +297
// Because all chars are ASCII a slice of chars is also guaranteed to be valid slice of
// bytes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: I rekon its better without this comment. Mainly because 'all chars are ASCII' is not true if one interprets 'chars' as the Rust type. But also because the SOUNDNESS line already documents the code.

Copy link
Copy Markdown
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 2713e69

@tcharding
Copy link
Copy Markdown
Member

Nice PR.

Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 49818c6; successfully ran local tests

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.

3 participants