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

Changed ifstream open mode to binary for overmap and player JSON. #12962

Merged
merged 1 commit into from Jul 17, 2015

Conversation

Projects
None yet
4 participants
@DanmakuDan
Copy link
Contributor

commented Jul 16, 2015

Fixes #12945. I'm guessing you can't have a Windows file with long lines and \r\n and expect Windows to know where your stream pointer is. I think the 4 MB stream location where it fails is probably more than a coincidence, but sample programs can break the pointer with only a few bytes.

@kevingranade

This comment has been minimized.

Copy link
Member

commented Jul 16, 2015

@DanmakuDan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2015

I think the main difference on the Windows side is how binary/text mode deals with control characters. The JSON code already looks like it deals with them without depending on the input mode by parsing each byte.

@kevingranade kevingranade self-assigned this Jul 17, 2015

@kevingranade

This comment has been minimized.

Copy link
Member

commented Jul 17, 2015

Pulling to build, I can't test, but it's the right thing to do anyway so I'll go ahead ad pull it as long as it doesn't seem to break anything.

@karlnp

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2015

I tested it on mingw64/win7, it works fine.

@kevingranade

This comment has been minimized.

Copy link
Member

commented Jul 17, 2015

Thanks for the confirmation :)

@kevingranade kevingranade merged commit ff8cff2 into CleverRaven:master Jul 17, 2015

1 check passed

default
Details
@chaosvolt

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2015

Ooh, I was about to wonder whether this actually did anything, but this was just now merged. Will be eager to see the results.

EDIT: Confirmed, build 3426 seems to be working fine to me. Thank you to DanmakuDan for this.

@kevingranade

This comment has been minimized.

Copy link
Member

commented Jul 17, 2015

Fixed #12945

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.