Bugfix read write compressed did not tell edian#26
Bugfix read write compressed did not tell edian#26Kiddinglife wants to merge 2 commits intoSLikeSoft:2.0.xfrom Kiddinglife:bugfix_read_write_compressed_did_not_tell_edian
Conversation
|
Thanks for the pull request. We'll review it shortly and then merge it into the repository. Looking at our CLA records, I realize we don't have you listed there yet. Could you quickly review the CLA at https://github.com/SLikeSoft/SLikeNet/blob/master/.github/CONTRIBUTING.md and send us a mail (in case you agree)? |
|
@Luke1410 has sent email to you lol. |
Luke1410
left a comment
Luke1410
left a comment
There was a problem hiding this comment.
I stopped the review at this point for the time being. Discussing the issue first in comments.
| while ( currentByte > 0 ) | ||
| // From high byte to low byte, if high byte is a byteMatch then write a 1 bit. | ||
| // Otherwise write a 0 bit and then write the remaining bytes | ||
| if (!IsNetworkOrder) |
There was a problem hiding this comment.
Also incorrect usage --- Endian swapping is conditionally controlled by the __BITSTREAM_NATIVE_END macro - should call DoEndianSwap() instead
| if (inByteArray[currentByte] == byteMatch) // If high byte is byteMatch (0 of 0xff) then it would have the same value shifted | ||
| { | ||
| Write(true); | ||
| currByte--； |
There was a problem hiding this comment.
should be currentByte I take it
also invalid character used (0x203a) instead of ;
| if (inByteArray[currentByte] == byteMatch) // If high byte is byteMatch (0 of 0xff) then it would have the same value shifted | ||
| { | ||
| Write(true); | ||
| currByte++； |
There was a problem hiding this comment.
cyrrByte -> currentByte?
invalid character instead of simple ;
|
|
||
| currentByte--; | ||
| // make sure we are now on the lowest byte (index highest) | ||
| if (currByte < ((size >> 3) - 1)) |
| return ; | ||
| // make sure we are now on the lowest byte (index 0) | ||
| if (currentByte > 0) | ||
| return false; |
There was a problem hiding this comment.
this is a void function - should be simply return?
| currentByte--; | ||
| // make sure we are now on the lowest byte (index highest) | ||
| if (currByte < ((size >> 3) - 1)) | ||
| return false; |
There was a problem hiding this comment.
void function - should be return?
| // Write the remainder of the data after writing 0 | ||
| Write(false); | ||
| // Write the remainings | ||
| WriteBits(src + currentByte, size - (currentByte << 3)); |
There was a problem hiding this comment.
src is undefined --- I guess you meant inByteArray instead?
| { | ||
| // Read the rest of the bytes | ||
|
|
||
| if (ReadBits(inOutByteArray, size - (currentByte << 3) == false) |
|
The two versions of Read/WriteCompressed() you are referencing are private functions in BitStream and explicitly denoted to assume native types (i.e. they are not designed to be called directly on non-native types). Do you have a specific case where this is causing problems for you and/or did you run into some issue with SLikeNet/RakNet where you suspected that the cause would be these two methods not handling endian swapping themselves? For example see the usage in BitStream::WriteCompressed(const templateType &inTemplateVar). This method calls the ReverseBytes() if required for endian swapping before calling WriteCompressed(unsigned char*, unsigned int, bool). So this looks correct to me. Do you see it otherwise? |
|
Hi @Luke1410 Let us go to the point. You said: So this looks correct to me. Do you see it otherwise? Anyway, ignoring my changes and safely closing this PR works no problems on me. Regards, |
|
I'll keep this PR open for the time being. There are certainly a few things I'd like to pick up from your suggestion here. If you wanna improve the pull request to a level where it can get merged without causing issues, don't hesitate to do so. Otherwise, as stated before, we'll get back to this one at a later point then. |
2 participants
low byte is stored in low memory address in little-endian system like windows
but low byte is stored in high memory address in big-endian system.
However, readcompressed() and writecompressed() do not take it into account. It is only working on little-endian system but will not work on big-endian system.
I am happy to discuss this further with someone who disagrees. ^_^