-
Notifications
You must be signed in to change notification settings - Fork 67
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
Refactor ChunkCompression
to use BinaryReader
and BinaryWriter
#1162
Refactor ChunkCompression
to use BinaryReader
and BinaryWriter
#1162
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but I cannot review this. This PR is in a state where remaking your changes would be easier for me than reviewing them.
If you move around so much code and do changes to it, then I cannot see what you actually changed, making it impossible to review.
If you want to move code, then please either make it in a separate PR, or at least split it into two commits, one commit that just moves the code to the place you want it (and it doesn't even have to compile), and one commit to apply all the changes to it.
Also if you do want separate function for each compression algorithm, then the compress and decompress functions should be right next to each other in the code, to make it easier to edit them in pairs.
Ok, I'll move them back into the switch and submit separate rearrangement PR after this one is merged |
I'm seeing double free error, but I have no idea where it happens :C
|
Did you try running it in debug mode? |
Yeah, well, I did it right now and there is no error :P |
Well if nothing works, you could always just use an external debugger |
So, I think I just compiled in dirty workspace after I merged master, after clean rebuild issue is gone. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me.
Related to: #1156