andrey-zherikov / argparse

Parser for command-line arguments
https://andrey-zherikov.github.io/argparse/
Boost Software License 1.0
30 stars 6 forks source link

Reduce allocations in Styling API #134

Closed SirNickolas closed 9 months ago

SirNickolas commented 9 months ago
  1. TextStyle stores its sequence as a string instead of ubyte[]. Thus it is allocated only once, not on every opCall.
  2. appender + std.conv.toChars are used instead of to!string.
  3. Primitive styles (bold, italic, etc.) are cached in global constants.
  4. ~= is used instead of ~ where possible.
  5. Added another overload of TextStyle.opCall, which writes to an output range. This allows to build strings with no intermediate allocations (see the last unit test for an example).

I have a question. Why do we need StyledText? As far as I can tell, the only place where it would be beneficial is an overload of getUnstyledTextLength(StyledText). But… it is not called from anywhere besides its unit test. I tried removing StyledText, and everything I had to do to deal with it was dropping several .toString() calls.

codecov[bot] commented 9 months ago

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (e72cb56) 98.43% compared to head (a60213b) 98.40%.

:exclamation: Current head a60213b differs from pull request most recent head 54f4b8a. Consider uploading reports for the commit 54f4b8a to get more accurate results

Files Patch % Lines
source/argparse/ansi.d 98.07% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## 2.x #134 +/- ## ========================================== - Coverage 98.43% 98.40% -0.03% ========================================== Files 25 25 Lines 1721 1759 +38 ========================================== + Hits 1694 1731 +37 - Misses 27 28 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

SirNickolas commented 9 months ago

I thought about making a separate method instead of overloading opCall (that would simplify StyleImpl as well), but how do we name it? bold.writeTo(sink, text);? formatTo? Something else? Plain write/format seem odd to me because it makes more sense to have such methods declared on the sink.

andrey-zherikov commented 9 months ago

I have a question. Why do we need StyledText? As far as I can tell, the only place where it would be beneficial is an overload of getUnstyledTextLength(StyledText). But… it is not called from anywhere besides its unit test. I tried removing StyledText, and everything I had to do to deal with it was dropping several .toString() calls.

TBH I don't remember why it was needed. If you can remove it without affecting public interface - go ahead (ideally in separate PR).

andrey-zherikov commented 9 months ago

I thought about making a separate method instead of overloading opCall (that would simplify StyleImpl as well), but how do we name it? bold.writeTo(sink, text);? formatTo? Something else? Plain write/format seem odd to me because it makes more sense to have such methods declared on the sink.

Could you clarify what you want to achieve? If you want to replace opCall with something then how will this code look like: "argument "~bold.("name")~" is bold")?

SirNickolas commented 9 months ago

Realized there is a more flexible approach: turn StyledText into a range of characters. It’ll definitely go into its own PR; we can discuss in #135 how exactly it should be done. In the meantime, I reverted the addition of new functionality.