Refactor the inner logic of the crate to cleaner, faster and zero-copy code #12
Conversation
|
|
||
| #[derive(Debug, Clone, Default)] | ||
| pub struct Slice<'s> { | ||
| pub source: &'s str, |
There was a problem hiding this comment.
In one of my projects I'm generating this from a Path, so I'd need to allocate anyway. Kinda. We could make it generic over Display, but that would probably end up spreading generic parameters everywhere. I can allocate everything beforehand and just pass the reference in here, so it's not a blocker. Just wanted to comment about my use case.
src/slice.rs
Outdated
| pub source: &'s str, | ||
| pub line_start: Option<usize>, | ||
| pub origin: Option<&'s str>, | ||
| pub annotations: Vec<SourceAnnotation<'s>>, |
There was a problem hiding this comment.
I guess this should be &'s [SourceAnnotation<'s>] since it's always user-supplied and not computed
|
|
||
| impl<'d> From<&Slice<'d>> for DisplayList<'d> { | ||
| fn from(slice: &Slice<'d>) -> Self { | ||
| let mut body = vec![]; |
There was a problem hiding this comment.
why allocate all this and not just render it lazily in the Display impl?
There was a problem hiding this comment.
The reason to separate building displaylist from formatting (Display) is that we need to compute the layout of the output before we draw it.
If I understand your question, you're saying that we could just take the Slice/Snippet and write the output, but due to the nature of the library, we need to perform a lot of computations to layout the structure where the decisions about the first line depend on the information in the last line etc.
Therefore, we need a "two pass" approach I believe. We can combine those two passes in one method, but then we limit ourselves in several ways.
My take on the Slice -> DisplayList -> Output is that it allows me to design a layout builder (Slice -> DisplayList) and separately a "rendering" code (DisplayList -> Output).
In the simplest use case, we can differentiate between "themes" that will take the same DisplayList and draw it differently (color, no color, rich unicode characters to draw tables/frames etc.) and we can also have different layouts ('rustc-like, elm-like` etc.).
I understand that it looks like it introduces cost in form of allocation, and I'd like to look for ways to minimize it, but I believe the clean codebase, separation of concerns, and ability to customize those two aspects justify the logic.
Does it sound reasonable to you?
There was a problem hiding this comment.
Yes totally. We should document this though ;) I was mostly curious as to why this was happening.
src/display_list/line.rs
Outdated
| let indent = if start == &0 { 0 } else { start + 1 }; | ||
| write!(f, "{:>1$}", "", indent)?; | ||
| if start == &0 { | ||
| write!(f, "{:_>1$}", "^", end - start + 1)?; |
There was a problem hiding this comment.
I don't think _ is a valid instruction for the format microlanguage, did you mean -?
Also, is this condition wrong?
| #[cfg(not(feature = "unicode"))] | ||
| impl MarkKind { | ||
| pub fn get(t: MarkKind) -> char { | ||
| match t { | ||
| MarkKind::Vertical => '|', | ||
| MarkKind::Horizontal => '-', | ||
| MarkKind::DownRight => '/', | ||
| MarkKind::UpRight => '|', | ||
| MarkKind::UpLeft => '^', | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(feature = "unicode")] | ||
| impl MarkKind { | ||
| pub fn get(t: MarkKind) -> char { | ||
| match t { | ||
| MarkKind::Vertical => '│', | ||
| MarkKind::Horizontal => '─', | ||
| MarkKind::DownRight => '┌', | ||
| MarkKind::UpRight => '└', | ||
| MarkKind::UpLeft => '┘', | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think this kind of global selection is the correct use of cargo compilation features. This should be a choice at the point of rendering, not implicitly changed to use box drawing characters if anyone in the compilation graph turns on the feature.
There was a problem hiding this comment.
yep, I'm just testing if the API works at all to get such features. I won't land it as such.
| #[cfg(feature = "ansi_term")] | ||
| use crate::renderers::ascii_default::styles::color::Style; | ||
| #[cfg(feature = "termcolor")] | ||
| use crate::renderers::ascii_default::styles::color2::Style; | ||
| #[cfg(all(not(feature = "ansi_term"), not(feature = "termcolor")))] | ||
| use crate::renderers::ascii_default::styles::plain::Style; |
There was a problem hiding this comment.
This global compile time feature selection is a misuse of cargo features. If I turn on both the ansi_term and termcolor features, this will fail to compile.
Features should be purely additive. Turning on features gives more options, not changes the behavior of code that works both with and without the feature.
There was a problem hiding this comment.
I agree those should not be mutually exclusive. Similarly to renderer choices, I'll move that to be a selector with options enabled via features.
|
As this is very out of date, I've moved this to #103. |
…hon-5.x chore(deps): update actions/setup-python action to v5
This is just the beginning, but already functional.
I got the
formatexample to mostly work, which allows me to compare the benchmark and it gets me57us -> 4.5uson my machine.I'm not sure if I'll stick to the
Displaytrait everywhere, since I want to maintain the pluggableformattersfor color/no-color and potentially richer formatters of the display list in the future, but the core logic is here.I still need to re-add a lot of features, which I'll be working on next.
If you have feedback on the proposed logic, or suggestions on how to make things lighter and faster, please, take a peak at the PR :)