3 ms·
That looks very hacky. Is there a reason for this limitation that you've found makes sense?
by Sirenos 3y ago
That looks very hacky. Is there a reason for this limitation that you've found makes sense?
- otikik 3y agoIt seems relatively easy to send a PR to allow supporting any number of spaces (or at least increase the upper limit to 16).
- messe 3y 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()
- ziml77 3y agoThe [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);
- messe 3y agoI'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.
- heliostatic 3y agoClosing the loop, this is now implemented and on main, thanks to msdrigg: https://github.com/helix-editor/helix/pull/7429 https://github.com/helix-editor/helix/pull/7429
- sph 3y agoThat 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.
- xojoc 3y agoMaybe it's done like that for performance reasons?
- messe 3y agoI'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.
- bobbyskelton41 3y agoIt 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.
- mat_epice 3y agoSomething something logical fallacy [1]. [1] https://en.m.wikipedia.org/wiki/Ad_hominem https://en.m.wikipedia.org/wiki/Ad_hominem
- unshavedyak 3y agoI 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.
- msdrigg 3y agoIts 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 3y agoThe 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 3y agoA 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 3y agoThere’s a dozen ways to solve this… from arc to ref cell, the language provides all sorts of methods to solve this.