Correctly handle bitshifting if system does not support unaligned a… #4366
Conversation
normanmaurer
commented
Oct 16, 2015
normanmaurer
commented
|
@nmittler @Scottmitch PTAL |
normanmaurer
force-pushed
the
unalligned_bug
branch
from
de74d0c
to
cb95053
Compare
| */ | ||
|
|
||
| package io.netty.buffer; | ||
|
|
|
lgtm |
normanmaurer
force-pushed
the
unalligned_bug
branch
from
cb95053
to
29a4a7e
Compare
|
|
||
| static short getShort(long address) { | ||
| if (UNALIGNED) { | ||
| short v = PlatformDependent.getShort(address); |
There was a problem hiding this comment.
Should we just update the PlatformDependent methods to do this for us?
There was a problem hiding this comment.
@nmittler nope we need to do this outside of it as we need the "raw" memory access for UnsafeDirectSwappedByteBuf (which had a bug because of this as well).
There was a problem hiding this comment.
@normanmaurer ok SGTM. Thanks!
|
@normanmaurer LGTM ... just one comment. |
normanmaurer
changed the title
|
@nmittler @Scottmitch actually there is a bug in the impl.... Let me fix it first 👍 . |
normanmaurer
force-pushed
the
unalligned_bug
branch
from
29a4a7e
to
a87348f
Compare
|
@Scottmitch @nmittler please check again. We need to always operate on BigEndian order here. Now it's correct. |
|
@normanmaurer looks like you rebased ... what exactly did you change? |
|
@nmittler +1 - I was about to ask the same thing. What does |
|
All the methods here need to set values in big endian order and return values in big endian order as our buffers use big endian by default (just like ByteBuffer). The converting to little endian only happens in the SwappedByteBuf implementations.
|
normanmaurer
force-pushed
the
unalligned_bug
branch
from
a87348f
to
2c6176a
Compare
| } | ||
|
|
||
| static int getUnsignedMedium(long address) { | ||
| return (PlatformDependent.getByte(address) & 0xff) << 16 | |
There was a problem hiding this comment.
if UNALIGNED we could reduce the number of PlatformDependent calls (and shifting / masking / ORing) by using PlatformDependent.getShort(..).
There was a problem hiding this comment.
Actually we don't need to check here, since getShort already does the right thing if UNALIGNED.
There was a problem hiding this comment.
@nmittler - Are you talking about re-using UnsafeByteBufUtil.getShort(..)? I was thinking of staying consistent with the rest of the methods in this class and directly using PlatformDependent.
There was a problem hiding this comment.
Yes, I meant calling UnsafeByteBufUtil.getShort ... either way works.
|
@normanmaurer - LGTM. Be good once we get some tests up an running on the different Endian and non-unaligned machines. |
|
@normanmaurer lgtm |
…ccess. Motivation: We had a bug in our implemention which double "reversed" bytes on systems which not support unaligned access. Modifications: - Correctly only reverse bytes if needed. - Share code between unsafe implementations. Result: No more data-corruption on sytems without unaligned access.
normanmaurer
force-pushed
the
unalligned_bug
branch
from
2c6176a
to
bb3b099
Compare
| } | ||
| } | ||
|
|
||
| static void setMedium(long address, int value) { |
There was a problem hiding this comment.
@nmittler please check
|
@nmittler please check updated PR... Please take special care when review |
