Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose fromStrict/toStrict directly from Data.ByteString #281

Merged
merged 1 commit into from
Sep 15, 2020

Conversation

Bodigrim
Copy link
Contributor

@Bodigrim Bodigrim commented Sep 8, 2020

Closes #279.

Basically I just moved fromStrict / toStrict from Data.ByteString.Lazy to Data.ByteString.Lazy.Internal. The difficulty is that their implementation depends on Data.ByteString.{length,null,take,drop}. I could potentially move these four functions to Data.ByteString.Internal, but I decided just to operate on fields of BS directly. IMHO it is a better option, because it moves less stuff between modules and also shaves off a couple of picoseconds, avoiding conditional branches in take / drop.

Comment on lines +53 to +54
fromStrict, -- :: ByteString -> Lazy.ByteString
toStrict, -- :: Lazy.ByteString -> ByteString
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, in the context of the Data.ByteString module, these seem like better names to me:

Suggested change
fromStrict, -- :: ByteString -> Lazy.ByteString
toStrict, -- :: Lazy.ByteString -> ByteString
toLazy, -- :: ByteString -> Lazy.ByteString
fromLazy, -- :: Lazy.ByteString -> ByteString

Data.ByteString.Lazy should keep the old names though.

Thoughts?!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the new names better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels a bit weird to have the very same function under two different names. Is there any prior art?

Imagine having both modules in scope:

import qualified Data.ByteString as BS
import qualified Data.ByteString.Lazy as BL

If fromStrict is the same entity, just re-exported, GHC suggests only one option for a hole _ :: BS.ByteString -> BL.ByteString - I do not even have to search for this function in haddocks! But if we define toLazy = fromStrict in Data.ByteString, GHC would be obliged to suggest both options. Now this is really confusing for a user: he does not know that it is just synonyms, so needs to check haddocks for both modules and painstakingly compare semantics.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, that means that folks who just want to trim the import lists don't win. They have to change their code, and if they want a simple migration path to the new bytestring releases that make the imports optional, they've more work to do.

One might even provide both sets of names. fromStrict == toLazy and fromLazy == toStrict, and re-export both forms from the Internal module. So you can have either name from either source.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reason for proposing these names was that I think that with Data.ByteString.toStrict and Data.ByteString.fromStrict it's not obvious which type we're respectively converting from or to. The other type could plausibly be [Word8] or String.

However, that means that folks who just want to trim the import lists don't win. They have to change their code, and if they want a simple migration path to the new bytestring releases that make the imports optional, they've more work to do.

@vdukhovni Can you expand on the scenario that you're describing here?

I don't understand how anyone could profit from the new {from,to}Strict re-exports while retaining compatibility with older bytestring versions.

@Bodigrim
Copy link
Contributor Author

Bodigrim commented Sep 9, 2020

Let's move the discussion about toLazy / fromLazy to the top-level thread.

I don't like having multiple names for the same thing, because it tends to lead to a chaos. Imagine importing both flavours of ByteString:

import qualified Data.ByteString as BS
import qualified Data.ByteString.Lazy as BL

Now for each conversion you have a choice between BS.toLazy / BL.fromStrict. If the module is large and is a collaborative effort (and especially if code is moved between modules), soon enough you'll end up having them both in an inconsistent fashion. This does not look appealing to me.


At the moment I tend to write

import qualified Data.ByteString as B
import qualified Data.ByteString.Lazy as B (fromStrict, toStrict)

If fromStrict and toStrict are exported directly from Data.ByteString, I can just remove the second import.

Copy link
Member

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the re-exports make sense, even though {from,to}Lazy might be clearer names in the context of Data.ByteString.

If there's demand for adding the latter names, we can still do so. I don't currently have the bandwidth to drive that change though.

@Bodigrim
Copy link
Contributor Author

If eveyone is ok with re-exports, I am keen to merge this before release.

@vdukhovni
Copy link
Contributor

If eveyone is ok with re-exports, I am keen to merge this before release.

Works for me. I don't think the "more natural" names actually pan out in practice, so keeping just one set visible from multiple places is roughly optimal.

@Bodigrim Bodigrim added this to the 0.11.0.0 milestone Sep 14, 2020
@Bodigrim Bodigrim merged commit 1ef4097 into haskell:master Sep 15, 2020
@Bodigrim Bodigrim deleted the export-fromStrict branch September 15, 2020 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose fromStrict/toStrict from Data.ByteString
3 participants