lsp icon indicating copy to clipboard operation
lsp copied to clipboard

use OsPath in NormalizedFilePath

Open kokobd opened this issue 2 years ago • 4 comments

Closes #445. Current status:

  • existing tests pass
  • breaks backward compatibility due to the necessity to add MonadThrow to the type of some functions
  • use filepath-compat to avoid CPP, and always use OsPath in NormalizedFilePath
  • provide two sets of functions for NormalizedFilePath. One for FilePath, another for OsPath

kokobd avatar Jul 25 '22 18:07 kokobd

refresh

✅ Pull request refreshed

mergify[bot] avatar Aug 14 '22 03:08 mergify[bot]

@Mergify refresh

kokobd avatar Aug 14 '22 03:08 kokobd

refresh

✅ Pull request refreshed

mergify[bot] avatar Aug 14 '22 03:08 mergify[bot]

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

kokobd avatar Aug 14 '22 03:08 kokobd

@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.

kokobd avatar Aug 29 '22 13:08 kokobd

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

michaelpj avatar Aug 31 '22 10:08 michaelpj

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.

hasufell avatar Aug 31 '22 14:08 hasufell

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)

kokobd avatar Aug 31 '22 15:08 kokobd