qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alan Conway" <acon...@redhat.com>
Subject Re: Review Request 33194: QPID-6470: Fix float conversion problems.
Date Wed, 15 Apr 2015 20:01:08 GMT


> On April 15, 2015, 3:04 a.m., Andrew Stitcher wrote:
> > I still really don't like the code changing depending on the endianness of the platform
- Wouldn't it just be better to read the value as either a 4 or 8 byte int (uint32_t or uint64_t)
and then just use a union to read the value as float or double?
> > ie
> > 
> > union {double d; uint64_t i} converter64;
> > union {float f; uint32_t i} converter32;
> > 
> > converter64.i = bytevalue;
> > return converter64.d;
> > 
> > etc.
> > 
> > - It's a bit of a hack but it is pretty portable as the alignment is required by
the standard (I'm pretty sure)
> > 
> > Then we can just get rid of copyConvert & convertIfRequired which would make
me happier.

I don't mind how we do the byte swapping as long as we only do it in one place. The current
situation where the shift logic is spread all over and we mix in the Endian stuff is bad.
 I'll clean it up.
I don't understand your attachment to shifting. The goal is to get unaligned network-order
bytes into an aligned machine-order location. I did some tests and the Endian for loop is
a terrible way to do it (much worse than shifting) but on linux byteswap.h and endian.h are
about twice as fast as shifting and Visual C++ also has fast byte-swap built-ins. I'm leaning
towards a portable shift implementation replaced by endian.h if available.


> On April 15, 2015, 3:04 a.m., Andrew Stitcher wrote:
> > trunk/qpid/cpp/src/tests/FieldValue.cpp, line 93
> > <https://reviews.apache.org/r/33194/diff/1/?file=927608#file927608line93>
> >
> >     Why shouldn't floats convert to ints? (unless they outside the representable
range)

They should. Originally they were "converting" by bitwise copy which was bad, but I can easily
convert them properly now.


- Alan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33194/#review80147
-----------------------------------------------------------


On April 14, 2015, 9:57 p.m., Alan Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33194/
> -----------------------------------------------------------
> 
> (Updated April 14, 2015, 9:57 p.m.)
> 
> 
> Review request for qpid and Andrew Stitcher.
> 
> 
> Repository: qpid
> 
> 
> Description
> -------
> 
> Previous code would incorrectly convert between float and int types producing nonsense
values,
> and would not allow legal conversions between float and double types.
> 
> Created FixedWidthIntValue and FixedWidthFloatValue template subclasses to correctly
> handle conversions. Enabled FieldValue unit tests for float conversions.
> 
> 
> Diffs
> -----
> 
>   trunk/qpid/cpp/src/qpid/framing/Endian.h 1673017 
>   trunk/qpid/cpp/src/qpid/framing/Endian.cpp 1673017 
>   trunk/qpid/cpp/src/qpid/framing/FieldValue.h 1673017 
>   trunk/qpid/cpp/src/qpid/framing/FieldValue.cpp 1673017 
>   trunk/qpid/cpp/src/tests/FieldTable.cpp 1673017 
>   trunk/qpid/cpp/src/tests/FieldValue.cpp 1673017 
> 
> Diff: https://reviews.apache.org/r/33194/diff/
> 
> 
> Testing
> -------
> 
> Added unit tests for float conversions.
> 
> 
> Thanks,
> 
> Alan Conway
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message