rnix-lsp icon indicating copy to clipboard operation
rnix-lsp copied to clipboard

Cleanup

Open nrdxp opened this issue 3 years ago • 8 comments

The biggest change here is the cleansing of all outstanding clippy lint warnings. I had to add an exception for clippy::cast_possible_truncation as the lsp_types::Position struct requires a u32, but we are currently building it from a usize.

I also ran a cargo fmt on the codebase, removed the implicit nix-direnv dependency on the envrc, added some rust development tools to the devshell and pulled rust from unstable to get a slightly newer version.

One of the lints corrected a scenario where a value's Drop destructor was never called, so there may be a miniscule chance that this was the root cause of #33.

nrdxp avatar Sep 11 '21 20:09 nrdxp

I like these changes. Thank you @nrdxp! I also agree with @Ma27's feedback :-)

aaronjanse avatar Sep 18 '21 22:09 aaronjanse

Just tried it out. Seems like a way to trigger the memleak is to open a let binding but not close it, then add an inherit. Instant trigger.

fufexan avatar Sep 19 '21 11:09 fufexan

Wait.... I think... I can reproduce the memleak, realiably :tada: (and I kinda feel dumb now :D)

I halted both affected rnix-lsp processes with gdb on my laptop for now since I'm actually busy with studying right now, but I'll try to take a look later, thanks! :tada:

Ma27 avatar Sep 19 '21 11:09 Ma27

Also, I'm willing to fix the merge conflict while merging this PR, rather than making @nrdxp fix it (unless they want to)

aaronjanse avatar Sep 26 '21 23:09 aaronjanse

@nrdxp may I ask if you intend to finish this? Otherwise I'd take care of it when i'll get to it :)

Ma27 avatar Nov 09 '21 17:11 Ma27

sorry, I completely forgot about this. I wouldn't mind rebasing it and finishing it off, but if you have the time and the motivation, feel free to take over. If not, I'll schedule it for this weekend. That said, it might be interesting to also bump the rust edition now that 2021 is stable, although it's a fairly minor change all things considered so it may not make much of a difference.

nrdxp avatar Nov 09 '21 21:11 nrdxp

If not, I'll schedule it for this weekend

If you still have the capacity, feel free to finish :) I wouldn't have managed to take care of it last week (and most likely also not this weekend).

. That said, it might be interesting to also bump the rust edition now that 2021 is stable, although it's a fairly minor change all things considered so it may not make much of a difference.

Oh, good point, thanks for the reminder!

Feel free to try it out :)

Ma27 avatar Nov 13 '21 13:11 Ma27

just fyi, I started on refactoring this on the weekend. Unfortunately didn't finish since I was rather busy with family stuff, but I'm hoping I should be able to get it done sometime in the next week.

nrdxp avatar Nov 16 '21 20:11 nrdxp