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

Is hSetBinaryMode required for ByteString I/O? #97

Closed
ghost opened this issue Nov 13, 2016 · 2 comments · Fixed by #305
Closed

Is hSetBinaryMode required for ByteString I/O? #97

ghost opened this issue Nov 13, 2016 · 2 comments · Fixed by #305

Comments

@ghost
Copy link

ghost commented Nov 13, 2016

This issue relates to the Haddock API documentation. Is the following note about Data.ByteString.Lazy.hGetContents correct or is it out of date?

Note: the Handle should be placed in binary mode with hSetBinaryMode for hGetContents to work correctly.

If it is out of date, please consider adding a statement somewhere in the documentation that hSetBinaryMode is definitely not required, for avoidance of confusion.

This also affects the following issue in the process package.

@sjakobi
Copy link
Member

sjakobi commented Jun 25, 2020

I suspect this might be related to #13. I believe the effects of hSetBinaryMode are currently simply not respected.

@Bodigrim
Copy link
Contributor

The note was added in 2b4485b, which points to https://gitlab.haskell.org/ghc/ghc/-/issues/3808 and leads to

bytestring/Data/ByteString.hs

Lines 1898 to 1914 in d79b997

hGetSome hh i
#if MIN_VERSION_base(4,3,0)
| i > 0 = createAndTrim i $ \p -> hGetBufSome hh p i
#else
| i > 0 = let
loop = do
s <- hGetNonBlocking hh i
if not (null s)
then return s
else do eof <- hIsEOF hh
if eof then return s
else hWaitForInput hh (-1) >> loop
-- for this to work correctly, the
-- Handle should be in binary mode
-- (see GHC ticket #3808)
in loop
#endif

These days bytestring requires base>=4.3, so hIsEOF is not used anymore and the warning about hSetBinaryMode does not apply. A PR removing #if MIN_VERSION_base(4,3,0) and warnings is very welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants