Hacker News Reader

Comment by Sirenos | Hacker News Reader

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

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.

Maybe it's done like that for performance reasons?

I'm struggling to see how that function could be a bottleneck, and if it were, memoization would easily get rid of it without the ridiculous limitation.

It could be better for sure, but that function has been there for almost as long as the project has been open-source if I recall correctly. It probably hasn't been touched since then because it wasn't a high priority.

Something something logical fallacy [1].

[1] https://en.m.wikipedia.org/wiki/Ad_hominem

I always find it interesting how hostile some (many?) peoples take on source code can be. Rather than give benefit of doubt, they attack. But even if it is some stupid code, some late-night, tired-brain, foggy-flu-delirium .. so what?

I get it, don't run a project if you see some bad code and question the quality of the project. Quality that very well could, and likely will, impact you. From bugs to performance. Don't run it, of course. But to attack it, seemingly with such personal zeal.. can people not improve?

I dunno, i guess i just don't feel the software we write on average is actually good. We make compromises and mistakes constantly. Our house is the purist of glass and many of us are just chucking stones like there's no tomorrow.

Lets take a step back. Breathe. Point out flaws by all means, but maybe reduce the often apparent revelry in someone else's mistake.

... sorry for the high horse rant. I'm nursing coffee and i've just seen it a lot with Lemmy/Kbin recently. Also this isn't pointed at anyone here directly.. just a pattern i feel i frequently see.

What? No. An ad hominem is "I think your argument is wrong because your face is stupid". "This is bad software because it looks like it's made by amateurs" isn't attacking their character, it's attacking their ability to write good software, which is highly relevant.

"The code is bad because it was written by a novice dev" sounds like an ad hominem attack to me (as opposed to "the code is bad because of X attribute of the code itself").

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.

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.

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

> 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()

The [0..n] slicing is what I was surprised to not see being done. As for what you're suggesting, using from_utf8 adds a overhead to check that you're giving it valid UTF-8 (this does not get optimized away). Dipping into unsafe can help here:

    const INDENTS: [u8; 256] = [b' '; 256];
    // ...
    IndentStyle::Spaces(n) => unsafe { std::str::from_utf8_unchecked(&INDENTS[0..n as usize]) },

But I would consider having INDENTS be a &str to get the cheap slicing without needing unsafe. Since putting in a string literal of 256 spaces is nasty I would use the const_format crate to generate the constant:

    const INDENTS: &str = str_repeat!(" ", 256);

I'm not sure I could justify bringing in a package for a one-liner, just to avoid an "unsafe" which could be easily documented in a comment as actually safe.

Closing the loop, this is now implemented and on main, thanks to msdrigg: https://github.com/helix-editor/helix/pull/7429

Nice!

This makes me happy, thanks for sharing

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.

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.

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.

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

Reply on news.ycombinator.com