qpid-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From acon...@apache.org
Subject svn commit: r1671125 - in /qpid/trunk/qpid/cpp/src: qpid/framing/Endian.cpp qpid/framing/Endian.h qpid/framing/FieldValue.cpp qpid/framing/FieldValue.h tests/FieldValue.cpp
Date Fri, 03 Apr 2015 18:47:04 GMT
Author: aconway
Date: Fri Apr  3 18:47:03 2015
New Revision: 1671125

URL: http://svn.apache.org/r1671125
Log:
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.

Modified:
    qpid/trunk/qpid/cpp/src/qpid/framing/Endian.cpp
    qpid/trunk/qpid/cpp/src/qpid/framing/Endian.h
    qpid/trunk/qpid/cpp/src/qpid/framing/FieldValue.cpp
    qpid/trunk/qpid/cpp/src/qpid/framing/FieldValue.h
    qpid/trunk/qpid/cpp/src/tests/FieldValue.cpp

Modified: qpid/trunk/qpid/cpp/src/qpid/framing/Endian.cpp
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/cpp/src/qpid/framing/Endian.cpp?rev=1671125&r1=1671124&r2=1671125&view=diff
==============================================================================
--- qpid/trunk/qpid/cpp/src/qpid/framing/Endian.cpp (original)
+++ qpid/trunk/qpid/cpp/src/qpid/framing/Endian.cpp Fri Apr  3 18:47:03 2015
@@ -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++) {

Modified: qpid/trunk/qpid/cpp/src/qpid/framing/Endian.h
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/cpp/src/qpid/framing/Endian.h?rev=1671125&r1=1671124&r2=1671125&view=diff
==============================================================================
--- qpid/trunk/qpid/cpp/src/qpid/framing/Endian.h (original)
+++ qpid/trunk/qpid/cpp/src/qpid/framing/Endian.h Fri Apr  3 18:47:03 2015
@@ -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();

Modified: qpid/trunk/qpid/cpp/src/qpid/framing/FieldValue.cpp
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/cpp/src/qpid/framing/FieldValue.cpp?rev=1671125&r1=1671124&r2=1671125&view=diff
==============================================================================
--- qpid/trunk/qpid/cpp/src/qpid/framing/FieldValue.cpp (original)
+++ qpid/trunk/qpid/cpp/src/qpid/framing/FieldValue.cpp Fri Apr  3 18:47:03 2015
@@ -232,7 +232,7 @@ void FieldValue::print(std::ostream& out
     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);
 }

Modified: qpid/trunk/qpid/cpp/src/qpid/framing/FieldValue.h
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/cpp/src/qpid/framing/FieldValue.h?rev=1671125&r1=1671124&r2=1671125&view=diff
==============================================================================
--- qpid/trunk/qpid/cpp/src/qpid/framing/FieldValue.h (original)
+++ qpid/trunk/qpid/cpp/src/qpid/framing/FieldValue.h Fri Apr  3 18:47:03 2015
@@ -106,12 +106,11 @@ class QPID_COMMON_CLASS_EXTERN FieldValu
   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() c
 
 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();

Modified: qpid/trunk/qpid/cpp/src/tests/FieldValue.cpp
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/cpp/src/tests/FieldValue.cpp?rev=1671125&r1=1671124&r2=1671125&view=diff
==============================================================================
--- qpid/trunk/qpid/cpp/src/tests/FieldValue.cpp (original)
+++ qpid/trunk/qpid/cpp/src/tests/FieldValue.cpp Fri Apr  3 18:47:03 2015
@@ -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()
 



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org


Mime
View raw message