Skip to content

Use EnumSet instead of a full-blown btreemap for the attributes #244

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 2 commits into from
Mar 8, 2025

Conversation

jwiesler
Copy link
Contributor

@jwiesler jwiesler commented Mar 5, 2025

This minimal change reduces the size of Style from 32 bytes to 10 and removes allocations needed to construct a style. This also means the remainder of the API could become const. If you want me to do that please ping me and I will do it in this mr.

The semantics of a bitset differ from the btreemap in so far as the btreemap is in insertion order, but I think this does not matter for attribute codes being printed, however, please verify this. Maybe this can be fixed by reordering the enum members.

The EnumSetType derive automatically derives Copy, Clone, PartialEq and Eq which is why I had to remove them.

For the library used see enumset.

@djc
Copy link
Member

djc commented Mar 6, 2025

This minimal change reduces the size of Style from 32 bytes to 10 and removes allocations needed to construct a style. This also means the remainder of the API could become const.

Which part of this matters to you (most), what is your use case/motivation for this PR?

IMO enumset is pretty large dependency for something low-level like console. In particular it pulls in all the proc-macro machinery which I would prefer to avoid.

Maybe a more minimal "manual" implementation of the same principles could make sense.

@jwiesler
Copy link
Contributor Author

jwiesler commented Mar 6, 2025

In short: Const API and no allocations/performance improvements.
Longer: I've been looking for a library that does this styling in an optimal way without first rendering the input to a string or something like this, which console does nicely by transparently implementing Debug and Display. However, e.g. a bold Style can't be stored in a constant, since it requires attr which is not const and this really doesn't have to be the case. Also storing this in a tree set on the heap seems overkill for this.

I could do a manual implementation with an u16 as storage (maybe 30-60 LOC), but that would make it harder for you to review :) Please tell me before I invest the time if you'd be willing to even merge this afterwards or under which constraints (e.g. no unsafe, ...).

@djc
Copy link
Member

djc commented Mar 6, 2025

Happy to review 30-60 LoC of manual implementation, if it doesn't contain unsafe and doesn't bring in new dependencies.

(Maybe also look at the anstyle family of crates, including anstyle-stream.)

@jwiesler
Copy link
Contributor Author

jwiesler commented Mar 6, 2025

I looked at anstyle-stream and implemented it in 60 LOC (barely). There are more changes, I added two tests and made all functions I found const.

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

Looks great, just a few small nits!

}

#[inline]
fn ansi_nums(self) -> impl Iterator<Item = u16> {
Copy link
Member

Choose a reason for hiding this comment

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

I'm inclined to leave this out here and inline it into the one (library) callsite. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it like this more, since bits would be a very opaque term on the calling site. But your call.

@jwiesler
Copy link
Contributor Author

jwiesler commented Mar 6, 2025

Phew: mutable references are not allowed in constant functions. That rust version 1.66 do be pretty old.

@djc
Copy link
Member

djc commented Mar 6, 2025

Phew: mutable references are not allowed in constant functions. That rust version 1.66 do be pretty old.

Can probably just take self like the other methods?

@jwiesler jwiesler force-pushed the main branch 2 times, most recently from 4a9758d to a0c6e46 Compare March 6, 2025 20:41
@jwiesler
Copy link
Contributor Author

jwiesler commented Mar 6, 2025

True, I thought those were the attr() functions.

@djc djc merged commit bc32acc into console-rs:main Mar 8, 2025
15 checks passed
@djc
Copy link
Member

djc commented Mar 8, 2025

This looks pretty good, I'll probably make some small tweaks.

@djc djc mentioned this pull request Mar 8, 2025
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.

2 participants