-
-
Notifications
You must be signed in to change notification settings - Fork 126
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
Conversation
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. |
In short: Const API and no allocations/performance improvements. 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, ...). |
Happy to review 30-60 LoC of manual implementation, if it doesn't contain (Maybe also look at the anstyle family of crates, including anstyle-stream.) |
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 |
There was a problem hiding this 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> { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Phew: |
Can probably just take |
4a9758d
to
a0c6e46
Compare
True, I thought those were the |
This looks pretty good, I'll probably make some small tweaks. |
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 becomeconst
. 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 derivesCopy
,Clone
,PartialEq
andEq
which is why I had to remove them.For the library used see enumset.