Skip to content
This repository has been archived by the owner on May 6, 2024. It is now read-only.

Fix extra newline at end of NLE run; fix fwrite when bz2 is disabled. #118

Merged
merged 1 commit into from
Nov 6, 2020

Conversation

heiner
Copy link
Contributor

@heiner heiner commented Nov 6, 2020

No description provided.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 6, 2020
@heiner heiner requested review from yobibyte and cdmatters November 6, 2020 10:53
Copy link
Contributor

@yobibyte yobibyte left a comment

Choose a reason for hiding this comment

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

Fix works for me (checked on fix-newline branch).

@@ -77,7 +77,7 @@ write_data(void *buf, int length)
BZ2_bzWrite(&bzerror, nle->ttyrec_bz2, buf, length);
assert(bzerror == BZ_OK);
#else
assert(fwrite(buf, 1, length, nle->ttyrec) == 0);
assert(fwrite(buf, 1, length, nle->ttyrec) == length);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain this line to me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes a mistake I made earlier: fwrite doesn't signal an OK status with 0, it returns the number of bytes written when everything went well.

This code path is only active when NLE_BZ2_TTYRECS is not defined, which I did to test out this change as it allows to use nle-ttyplay -p.

@heiner heiner merged commit cae0d3e into master Nov 6, 2020
@samvelyan samvelyan deleted the fix-newline branch February 9, 2021 12:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants