QPID-6470: FieldValue::getFloatingPointValue() converts endian each time it is called.

When calling getFloatingPointValue multiple times, the octets are endian-converted each time.
Actually we need to make a copy first and then call convertIfRequired().

This fix is from a pull request by Pavel Pokutnev (see the JIRA).
commit 4ed0ce9c9b74b136c49735b19efb80489aa495a3

His original patch was correct, I made some additions:

- Added a unit test: qpid/cpp/src/tests/FieldValue.cpp
- Fixed some incorrect uses of "const" in nearby code.
- Replaced a for loop with std::copy, more readable and more optimizable.

There are still serious problems with float conversion shown up by the unit tests,
the relevant tests are commented out till these issues are fixed.

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk/qpid@1671125 13f79535-47bb-0310-9956-ffa450edef68

5 files changed, 45 insertions, 43 deletions

diff --git a/cpp/src/qpid/framing/Endian.cpp b/cpp/src/qpid/framing/Endian.cpp
index 5acc3c459f..decfe4b2d9 100644
--- a/cpp/src/qpid/framing/Endian.cpp
+++ b/cpp/src/qpid/framing/Endian.cpp
@@ -35,7 +35,7 @@ bool Endian::testBigEndian()
     return a == b;
 }
 
-uint8_t* Endian::convertIfRequired(uint8_t* const octets, int width)
+uint8_t* Endian::convertIfRequired(uint8_t* octets, int width)
 {
     if (instance.littleEndian) {
         for (int i = 0; i < (width/2); i++) {
diff --git a/cpp/src/qpid/framing/Endian.h b/cpp/src/qpid/framing/Endian.h
index 077d5a3e9b..dea8eaa3b7 100644
--- a/cpp/src/qpid/framing/Endian.h
+++ b/cpp/src/qpid/framing/Endian.h
@@ -34,7 +34,7 @@ namespace framing {
 class Endian
 {
   public:
-    static uint8_t* convertIfRequired(uint8_t* const octets, int width);
+    static uint8_t* convertIfRequired(uint8_t* octets, int width);
   private:
     const bool littleEndian;
     Endian();
diff --git a/cpp/src/qpid/framing/FieldValue.cpp b/cpp/src/qpid/framing/FieldValue.cpp
index 4abed0f77f..1d39c57edf 100644
--- a/cpp/src/qpid/framing/FieldValue.cpp
+++ b/cpp/src/qpid/framing/FieldValue.cpp
@@ -232,7 +232,7 @@ void FieldValue::print(std::ostream& out) const {
     out << ')';
 }
 
-uint8_t* FieldValue::convertIfRequired(uint8_t* const octets, int width)
+uint8_t* FieldValue::convertIfRequired(uint8_t* octets, int width)
 {
     return Endian::convertIfRequired(octets, width);
 }
diff --git a/cpp/src/qpid/framing/FieldValue.h b/cpp/src/qpid/framing/FieldValue.h
index 1adcb2fa07..94126df053 100644
--- a/cpp/src/qpid/framing/FieldValue.h
+++ b/cpp/src/qpid/framing/FieldValue.h
@@ -106,12 +106,11 @@ class QPID_COMMON_CLASS_EXTERN FieldValue {
   protected:
     FieldValue(uint8_t t, Data* d): typeOctet(t), data(d) {}
 
-    QPID_COMMON_EXTERN static uint8_t* convertIfRequired(uint8_t* const octets, int width);
+    QPID_COMMON_EXTERN static uint8_t* convertIfRequired(uint8_t* octets, int width);
 
   private:
     uint8_t typeOctet;
     std::auto_ptr<Data> data;
-
 };
 
 template <>
@@ -219,12 +218,13 @@ inline T FieldValue::getIntegerValue() const
 
 template <class T, int W>
 inline T FieldValue::getFloatingPointValue() const {
-    FixedWidthValue<W>* const fwv = dynamic_cast< FixedWidthValue<W>* const>(data.get());
+    const FixedWidthValue<W>* fwv = dynamic_cast<FixedWidthValue<W>*>(data.get());
     if (fwv) {
         T value;
-        uint8_t* const octets = convertIfRequired(fwv->rawOctets(), W);
-        uint8_t* const target = reinterpret_cast<uint8_t*>(&value);
-        for (size_t i = 0; i < W; ++i) target[i] = octets[i];
+        uint8_t* target = reinterpret_cast<uint8_t*>(&value);
+        const uint8_t* octets = fwv->rawOctets();
+        std::copy(octets, octets + W, target);
+        convertIfRequired(target, W);
         return value;
     } else {
         throw InvalidConversionException();
diff --git a/cpp/src/tests/FieldValue.cpp b/cpp/src/tests/FieldValue.cpp
index 0ebd0d7d44..34faee2953 100644
--- a/cpp/src/tests/FieldValue.cpp
+++ b/cpp/src/tests/FieldValue.cpp
@@ -2,7 +2,7 @@
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements.  See the NOTICE file
  * distributed with this work for additional information
- * regarding copyright ownership.  The ASF licenses this file
+ uni* regarding copyright ownership.  The ASF licenses this file
  * to you under the Apache License, Version 2.0 (the
  * "License"); you may not use this file except in compliance
  * with the License.  You may obtain a copy of the License at
@@ -19,6 +19,7 @@
 #include "qpid/framing/FieldValue.h"
 
 #include "unit_test.h"
+#include <boost/test/floating_point_comparison.hpp>
 
 namespace qpid {
 namespace tests {
@@ -29,9 +30,8 @@ using namespace qpid::framing;
 
 Str16Value s("abc");
 IntegerValue i(42);
-//DecimalValue d(1234,2);
-//FieldTableValue ft;
-//EmptyValue e;
+FloatValue f(42.42);
+DoubleValue df(123.123);
 
 QPID_AUTO_TEST_CASE(testStr16ValueEquals)
 {
@@ -43,52 +43,54 @@ QPID_AUTO_TEST_CASE(testStr16ValueEquals)
     BOOST_CHECK(s.convertsTo<int>() == false);
     BOOST_CHECK(s.get<std::string>() == "abc");
     BOOST_CHECK_THROW(s.get<int>(), InvalidConversionException);
-//    BOOST_CHECK(s != ft);
 
 }
 
 QPID_AUTO_TEST_CASE(testIntegerValueEquals)
 {
+    BOOST_CHECK(i.get<int>() == 42);
     BOOST_CHECK(IntegerValue(42) == i);
     BOOST_CHECK(IntegerValue(5) != i);
     BOOST_CHECK(i != s);
     BOOST_CHECK(i.convertsTo<std::string>() == false);
+    BOOST_CHECK(i.convertsTo<float>() == false);
     BOOST_CHECK(i.convertsTo<int>() == true);
     BOOST_CHECK_THROW(i.get<std::string>(), InvalidConversionException);
-    BOOST_CHECK(i.get<int>() == 42);
-//    BOOST_CHECK(i != ft);
-}
 
-#if 0
-QPID_AUTO_TEST_CASE(testDecimalValueEquals)
-{
-    BOOST_CHECK(DecimalValue(1234, 2) == d);
-    BOOST_CHECK(DecimalValue(12345, 2) != d);
-    BOOST_CHECK(DecimalValue(1234, 3) != d);
-    BOOST_CHECK(d != s);
+    //FIXME aconway 2015-04-03: fails
+    //BOOST_CHECK_THROW(i.get<float>(), InvalidConversionException);
 }
 
-QPID_AUTO_TEST_CASE(testFieldTableValueEquals)
+QPID_AUTO_TEST_CASE(testFloatValueEquals)
 {
-    ft.getValue().setString("foo", "FOO");
-    ft.getValue().setInt("magic", 7);
-
-    BOOST_CHECK_EQUAL(std::string("FOO"),
-                            ft.getValue().getString("foo"));
-    BOOST_CHECK_EQUAL(7, ft.getValue().getInt("magic"));
-
-    FieldTableValue f2;
-    BOOST_CHECK(ft != f2);
-    f2.getValue().setString("foo", "FOO");
-    BOOST_CHECK(ft != f2);
-    f2.getValue().setInt("magic", 7);
-    BOOST_CHECK_EQUAL(ft,f2);
-    BOOST_CHECK(ft == f2);
-    f2.getValue().setString("foo", "BAR");
-    BOOST_CHECK(ft != f2);
-    BOOST_CHECK(ft != i);
+    // FIXME aconway 2015-04-03: The commented out tests are bug QPID-6470.
+    // The basic problems are:
+    // - allows meaningles conversion between int and float types.
+    // - does not allow expected conversion between float and double types.
+
+    // BOOST_CHECK(f.convertsTo<float>() == true);
+    BOOST_CHECK_EQUAL(FloatValue(42.42), f);
+    BOOST_CHECK_CLOSE(f.get<float>(), 42.42, 0.001);
+    // Check twice, regression test for QPID-6470 where the value was corrupted during get.
+    BOOST_CHECK_EQUAL(FloatValue(42.42), f);
+    BOOST_CHECK_CLOSE(f.get<float>(), 42.42, 0.001);
+
+    // Float to double conversion
+    // BOOST_CHECK(f.convertsTo<double>() == true);
+    // BOOST_CHECK_CLOSE(f.get<double>(), 42.42, 0.001); 
+
+    // Double value
+    BOOST_CHECK_CLOSE(df.get<double>(), 123.123, 0.001);
+    // BOOST_CHECK(f.convertsTo<float>() == true);
+    // BOOST_CHECK(f.convertsTo<double>() == true);
+    // BOOST_CHECK_CLOSE(df.get<float>(), 123.123, 0.001);
+
+    // Invalid conversions should fail.
+    BOOST_CHECK(!f.convertsTo<std::string>());
+    // BOOST_CHECK(!f.convertsTo<int>());
+    BOOST_CHECK_THROW(f.get<std::string>(), InvalidConversionException);
+    // BOOST_CHECK_THROW(f.get<int>(), InvalidConversionException);
 }
-#endif
 
 QPID_AUTO_TEST_SUITE_END()