Skip to content

Make get :: Get Char total#70

Merged
kolmodin merged 1 commit into
haskell:masterfrom
ttuegel:total-char
Feb 23, 2015
Merged

Make get :: Get Char total#70
kolmodin merged 1 commit into
haskell:masterfrom
ttuegel:total-char

Conversation

@ttuegel
Copy link
Copy Markdown
Member

@ttuegel ttuegel commented Feb 20, 2015

The old Binary instance for Char used Data.Char.chr, which calls error
if given an invalid code point. The instance is modified to call fail
instead of error on invalid code points, which is total (within the Get
Monad). Calls to error inside Get will escape decodeOrFail.

This was discovered in Cabal-1.22.0.0, because we lean pretty heavily on binary now.

The old Binary instance for Char used Data.Char.chr, which calls error
if given an invalid code point. The instance is modified to call fail
instead of error on invalid code points, which is total (within the Get
Monad). Calls to error inside Get will escape decodeOrFail.
@dcoutts
Copy link
Copy Markdown
Contributor

dcoutts commented Feb 23, 2015

Right, we can either do this fix to avoid using the partial chr, or we could make the UTF8 decoder be more compliant so that it detects out of range characters itself and fails. This patch is ok in the interim though.

kolmodin added a commit that referenced this pull request Feb 23, 2015
Make get :: Get Char total

Fail within the Get monad for some UTF-8 errors
@kolmodin kolmodin merged commit ba3eb9d into haskell:master Feb 23, 2015
@kolmodin
Copy link
Copy Markdown
Member

Ok, let's do this in steps. I've opened #71 to track doing proper UTF8 decoding.

Thanks @ttuegel, I've merged your changes.

@kolmodin
Copy link
Copy Markdown
Member

Submitted to hackage as 0.7.4.0.

Comment thread src/Data/Binary/Class.hs
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please include the actual value so the user can debug their data.

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.

4 participants