haskell / lsp

Haskell library for the Microsoft Language Server Protocol
364 stars 91 forks source link

use OsPath in NormalizedFilePath #446

Closed kokobd closed 2 years ago

kokobd commented 2 years ago

Closes #445. Current status:

mergify[bot] commented 2 years ago

refresh

✅ Pull request refreshed

kokobd commented 2 years ago

@Mergify refresh

mergify[bot] commented 2 years ago

refresh

✅ Pull request refreshed

kokobd commented 2 years ago

I added a dimension to the test matrix, but Mergify is waiting for the old one. Strange.

kokobd commented 2 years ago

@michaelpj After some rewrites, I'm finally happy with the current version. The most notable change is that now we always store a UTF-8 encoded ShortByteString in NormalizedFilePath, so that toNormalizedFilePath :: FilePath -> NormalizedFilePath is still total.


And please only approve if you feel this is good. Let Mergify squash and merge it for us.

michaelpj commented 2 years ago

Oops, I missed the label! oh well, if you want to address any of the comments you can do a follow-up

hasufell commented 2 years ago

Looking plausible! I worry that maybe this extra encoding/decoding might lose some of the benchmark improvements you were getting?

Does this still use old directory/base for file operations? Because all of them do encoding/decoding at the FFI boundary. The only thing you'll gain is lower memory footprint due to avoiding String.

Can't say which one has the bigger impact.

kokobd commented 2 years ago

Does this still use old directory/base for file operations?

NormalizedFilePath in lsp-types doesn't use directory or some FilePath based IO functions. In HLS, we still use the old directory/base. (HLS can not upgrade before GHC 9.6)