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

Add IsList instances #219

Merged
merged 1 commit into from
Jul 4, 2020
Merged

Add IsList instances #219

merged 1 commit into from
Jul 4, 2020

Conversation

Bodigrim
Copy link
Contributor

@Bodigrim Bodigrim commented May 10, 2020

Resolves #106, as suggested by @dcoutts in #114 (and supersedes the latter).
CC @andrewthad

@sjakobi
Copy link
Member

sjakobi commented May 10, 2020

How does this behave for literals that don't fit into Word8, e.g.

[ 256 ] :: ByteString

or

[ -1 ] :: ByteString

?

@Bodigrim
Copy link
Contributor Author

Bodigrim commented May 10, 2020

Similar to Data.ByteString.pack.

[-1] throws warning: [-Woverflowed-literals] Literal -1 is out of the GHC.Word.Word8 range 0..255, as expected. Surprisingly enough, [256] does not throw such warning, when OverloadedLists are enabled, and silently returns "\NUL".

This seems to be a bug in GHC:

> [256] :: [Data.Word.Word8]

<interactive>:1:2: warning: [-Woverflowed-literals]
    Literal 256 is out of the GHC.Word.Word8 range 0..255
[0]
> :set -XOverloadedLists
> [256] :: [Data.Word.Word8]
[0]

@sjakobi
Copy link
Member

sjakobi commented May 10, 2020

@Bodigrim That looks worth reporting!

as suggested by @dcoutts in #116 (and supersedes the latter).

dcoutts didn't comment on #116. Did you mean a different issue?

@Bodigrim
Copy link
Contributor Author

Sure, filed as https://gitlab.haskell.org/ghc/ghc/issues/18172

Good catch. Sorry, I meant #114.

@andrewthad
Copy link
Contributor

The only thing arguably missing is a more efficient implementation of fromListN (rather than the default), but I don't really care one way or the other since I don't use overloaded lists.

@Bodigrim
Copy link
Contributor Author

I did not define fromListN, because it is difficult to make it both strictly more efficient than fromList and at the same time strictly conforming to the definition "The fromListN function takes the input list's length as a hint".

That said, I believe this is ready for review. @hvr @cartazio

@cartazio
Copy link

@Bodigrim im inclined that we should make FromList N validating. Rather than a hint. This was the take away about semantic mismatch on the libraries list. And since bytestring and vector are used in data loading contexts this is even more important!

@cartazio
Copy link

We should have a new discussion on the libraries list to discuss this. I think the hint semantic was the wrong choice at the time. ESP since the api then was about a desugaring trick rather than a user api

@Bodigrim
Copy link
Contributor Author

@cartazio sure, I remember that discussion on the libraries list and I totally agree that it does not make sense to treat n as a hint. But as long as this statement remains unchanged in GHC.Exts, I hesitate to ignore it.

I mean, anyone who cares about performance can reach for unsafePackLenBytes directly.

@cartazio
Copy link

I mean we should circulate the semantic change in docs on libraries list. But understood

@cartazio
Copy link

Wrt base

@Bodigrim
Copy link
Contributor Author

Bodigrim commented Jul 2, 2020

@hsyl20 could you please review?

@sjakobi sjakobi added this to the 0.10.12.0 milestone Jul 2, 2020
Copy link
Contributor

@hsyl20 hsyl20 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Bad docs for scanl
5 participants