top | item 36429531

(no title)

Sirenos | 2 years ago

That looks very hacky. Is there a reason for this limitation that you've found makes sense?

discuss

order

sph|2 years ago

That doesn't look hacky, it looks written by someone with 3 months of programming experience. I would personally avoid relying on an editor written by programming newbies.

tomjakubowski|2 years ago

It looks to me like it was written by someone quite intentionally to avoid allocations in the common case, and they punted on handling less common cases to later.

xojoc|2 years ago

Maybe it's done like that for performance reasons?

bobbyskelton41|2 years ago

Ad hominem attack aside... it was chosen because they didn't see a realistic need for more than 8 spaces. Nobody has openly complained in the past 2 years Helix has been around until now, so they were at least mostly right.

otikik|2 years ago

It seems relatively easy to send a PR to allow supporting any number of spaces (or at least increase the upper limit to 16).

messe|2 years ago

> allow supporting any number of spaces

As long as nothing is depending on 'static lifetime further down the line. (EDIT: ignore and see below)

> (or at least increase the upper limit to 16)

That should be doable for sure. Even something like (I'm not a rust developer, just have a passing familiarity, so my syntax may be off):

    IndentStyle::Spaces(n) if 0 < n && n <= 16 => &"                "[0..n as usize],
I think that would condense it all down into one line, rather than having special cases for all of 1 through 16.

EDIT: after toying around in the rust playground, I think the following will support any indent level (within the bounds of the u8 type).

At the top level:

    static INDENTS: [u8: 256] = [' '; 256];
And then in the as_str function:

    IndentStyle::Spaces(n) => std::str::from_utf8(&INDENTS[0..n as usize]).unwrap()

msdrigg|2 years ago

Its doing that to get a static str (no allocations). I assume it is for performance reasons, but I can’t speak to whether it’s actually a significant impact.

unshavedyak|2 years ago

The real flaw here in my mind is the lack of comments :D

If this was a carefully observed decision, one that is prone to looking odd, it deserves an explanation for future selves to not chase the rabbit of understanding.

tsukikage|2 years ago

A sane implementation would be for the fallback to construct a string containing the desired number of spaces on the fly and memoize. Unfortunately, this is written in Rust, and the language makes it super awkward to implement caching patterns transparently as they involve passing around references to mutable state.

bfrog|2 years ago

There’s a dozen ways to solve this… from arc to ref cell, the language provides all sorts of methods to solve this.