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.

Sign up for GitHub

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

Jump to bottom

Document possible interleaving of hPutStrLn. #201

Closed
wants to merge 1 commit into from
Closed

Document possible interleaving of hPutStrLn. #201

wants to merge 1 commit into from

Conversation

AndreasPK
Copy link
Contributor

See #200

sjakobi
sjakobi reviewed Jan 25, 2020
View reviewed changes
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.

Thanks! Documentation like this is very valuable!

Could you check that the resulting Haddocks look good? I suspect that an empty line after the first sentence could help.

Data/ByteString.hs Outdated Show resolved Hide resolved
sjakobi
sjakobi approved these changes Jan 25, 2020
View reviewed changes
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.

Cheers!

vdukhovni
vdukhovni approved these changes May 7, 2020
View reviewed changes
Copy link
Contributor

@vdukhovni vdukhovni left a comment

Choose a reason for hiding this comment

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

LGTM.
@cartazio

@vdukhovni vdukhovni mentioned this pull request May 8, 2020
Merged
Bodigrim
Bodigrim reviewed May 10, 2020
View reviewed changes
Data/ByteString.hs
@@ -1690,6 +1694,10 @@ putStr :: ByteString -> IO ()
putStr = hPut stdout

-- | Write a ByteString to stdout, appending a newline byte
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add a period, similar to the change in line 1682?

Data/ByteString.hs
@@ -1690,6 +1694,10 @@ putStr :: ByteString -> IO ()
putStr = hPut stdout

-- | Write a ByteString to stdout, appending a newline byte
--
-- This is not atomic.
Copy link
Contributor

@Bodigrim Bodigrim May 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It would be helpful to say what is atomic, to provide a hint for users interested to achieve this property. "Unlike putStr, this function is not atomic" or something along this.

Data/ByteString.hs
-- | Write a ByteString to a handle, appending a newline byte
-- | Write a ByteString to a handle, appending a newline byte.
--
-- This is not atomic.
Copy link
Contributor

@Bodigrim Bodigrim May 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It would be helpful to say what is atomic, to provide a hint for users interested to achieve this property. "Unlike hPutStr, this function is not atomic" or something along this.

Copy link
Contributor

Choose a reason for hiding this comment

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

This line might not be needed, because the following sentence explains the behaviour more conretely.

@chessai
Copy link
Member

chessai commented Jun 25, 2020

So, the test suite is failing on this PR for some reason. Does this need a rebase or something? From travis:

findSubstrings : [Failed]
*** Failed! Falsified (after 66 tests and 55 shrinks):
"aaa"
0
2
(used seed 3026286213326558307)

@vdukhovni
Copy link
Contributor

vdukhovni commented Jun 25, 2020 •
edited
Loading

Yes, it needs to be rebased, which will fix the problem. (This was one of the fixes I took care of when fixing all the outstanding CI issues). I'd still like to see #227 merged, which fixes testing bytestring versions that are newer than the bytestring dependency of the test frameworks. Without #227, tests fail to build forbytestring with the proposed #191 and GHC head.

Sitting on #227 is not a good idea. We should merge it, and if unexpectedly a minor issue is found, fix it. It lays a solid foundation for moving forward.

@sjakobi sjakobi linked an issue Jun 25, 2020 that may be closed by this pull request
hPutStrLn is not atomic #200
Open
@sjakobi sjakobi added this to the 0.10.12.0 milestone Jul 2, 2020
dcoutts
dcoutts approved these changes Aug 1, 2020
View reviewed changes
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

LGTM now too.

@dcoutts
Copy link
Contributor

dcoutts commented Aug 1, 2020

I'd rebase this on master myself but it's in another repo so I cannot.

@sjakobi sjakobi modified the milestones: 0.10.12.0, Soon Aug 19, 2020
@Bodigrim Bodigrim mentioned this pull request Aug 20, 2020
Merged
@Bodigrim
Copy link
Contributor

Rebased and merged in course of #259. Thanks @AndreasPK!

@Bodigrim Bodigrim closed this Aug 23, 2020
@Bodigrim Bodigrim modified the milestones: Soon, 0.11.0.0 Aug 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bodigrim Bodigrim Bodigrim left review comments

@fumieval fumieval fumieval left review comments

@sjakobi sjakobi sjakobi approved these changes

@dcoutts dcoutts dcoutts approved these changes

@vdukhovni vdukhovni vdukhovni approved these changes

Assignees
No one assigned
Labels
documentation
Projects
None yet
Milestone
0.11.0.0
Development

Successfully merging this pull request may close these issues.

hPutStrLn is not atomic
7 participants
@AndreasPK @chessai @vdukhovni @dcoutts @Bodigrim @fumieval @sjakobi