Merge pull request #310 from dawagner/optional-Werror-and-release-build

Add an option for removing Werror; have travis build in release config

Fix some compilation errors in release configuration (strict aliasing violations) and add an option to enable/disable the -Werror flag. This is useful for integrators.

Have travis build in release configuration instead of "None" configuration.

Author: krocard

.travis.yml

@@ -57,13 +57,13 @@ script:
         make -j &&
         CTEST_OUTPUT_ON_FAILURE=1 make ExperimentalTest ExperimentalMemCheck )
   - ( mkdir build && cd build &&
-        cmake -DCMAKE_PREFIX_PATH=$PREFIX -DCMAKE_INSTALL_PREFIX=../install .. &&
+        cmake -DCMAKE_PREFIX_PATH=$PREFIX -DCMAKE_INSTALL_PREFIX=../install -DCMAKE_BUILD_TYPE=Release .. &&
         make -j &&
         CTEST_OUTPUT_ON_FAILURE=1 make test &&
         make install &&
         cpack --verbose -G DEB && dpkg --info *.deb)
   - ( cd skeleton-subsystem &&
-        cmake -DCMAKE_INSTALL_PREFIX=../install . &&
+        cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=../install . &&
         make &&
         CTEST_OUTPUT_ON_FAILURE=1 make ExperimentalTest ExperimentalMemCheck &&
         make install )

CMakeLists.txt

@@ -42,7 +42,7 @@ option(DOXYGEN "Enable doxygen generation (you still have to run 'make doc')" OF
 option(REQUIREMENTS "Generate the html version of the 'requirements' documentation" OFF)
 option(PYTHON_BINDINGS "Python library to use the Parameter Framework from python" ON)
 option(C_BINDINGS "Library to use the Parameter Framework using a C API" ON)
-
+option(FATAL_WARNINGS "Turn warnings into errors (-Werror flag)" ON)
 
 # find and set the Parameter Framework's version
 execute_process(COMMAND git describe --tags --dirty
@@ -73,8 +73,14 @@ if(WIN32)
     # only public methods instead of the whole class, but they are too many to
     # do that. A separated plugin interface would fix that.
     set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /W4 /FIiso646.h -wd4127 -wd4251")
+
+    # FIXME: Once we have removed all warnings on windows, add the /WX flags if
+    # FATAL_WARNINGS is enabled
 else()
-    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -Werror -Wall -Wextra -Wconversion -Wno-sign-conversion")
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -Wall -Wextra -Wconversion -Wno-sign-conversion")
+    if(FATAL_WARNINGS)
+        set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror")
+    endif()
 endif()
 
 # Hide symbols by default, then exposed symbols are the same in linux and windows

bindings/python/CMakeLists.txt

@@ -50,7 +50,10 @@ find_package(PythonLibs ${PYTHON_VERSION_STRING} EXACT REQUIRED)
 include_directories(${PYTHON_INCLUDE_DIRS})
 
 set_property(SOURCE pfw.i PROPERTY CPLUSPLUS ON)
-set_property(SOURCE pfw.i PROPERTY SWIG_FLAGS "-Wall" "-Werror")
+set_property(SOURCE pfw.i PROPERTY SWIG_FLAGS "-Wall")
+if(FATAL_WARNINGS)
+    set_property(SOURCE pfw.i APPEND PROPERTY SWIG_FLAGS "-Werror")
+endif()
 
 swig_add_module(PyPfw python pfw.i)
 swig_link_libraries(PyPfw parameter ${PYTHON_LIBRARIES})
@@ -70,7 +73,8 @@ set_property(TARGET _PyPfw PROPERTY LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BIN
 # spurious anyway, as it relates to "ILogger *" which is an abstract
 # class/interface class and as such cannot be destroyed.
 # -Wno-error is set to prevent compilation failure in case of the code
-# generated by swig generates warnings
+# generated by swig generates warnings. We don't apply the FATAL_WARNING policy
+# here, since we consider this generated code as external.
 target_compile_definitions(_PyPfw PRIVATE SWIG_PYTHON_SILENT_MEMLEAK)
 set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-error")
 

parameter/FloatingPointParameterType.cpp

@@ -36,6 +36,7 @@
 #include <climits>
 #include "convert.hpp"
 #include "Utility.h"
+#include "BinaryCopy.hpp"
 
 using std::string;
 
@@ -138,7 +139,7 @@ bool CFloatingPointParameterType::toBlackboard(
             return false;
         }
 
-        float fData = reinterpret_cast<const float &>(uiValue);
+        auto fData = utility::binaryCopy<float>(uiValue);
 
         // Check against NaN or infinity
         if (!std::isfinite(fData)) {
@@ -172,9 +173,7 @@ bool CFloatingPointParameterType::toBlackboard(
         }
 
         // Move to the "raw memory" value space
-        // Using an intermediary reference variable to avoid klocwork false positive
-        const uint32_t &value = reinterpret_cast<const uint32_t &>(fValue);
-        uiValue = value;
+        uiValue = utility::binaryCopy<decltype(uiValue)>(fValue);
         return true;
     }
 }
@@ -193,8 +192,8 @@ void CFloatingPointParameterType::setOutOfRangeError(
         ostrStream << "real range [" << _fMin << ", " << _fMax << "]";
     } else {
 
-        uint32_t uiMin = reinterpret_cast<const uint32_t&>(_fMin);
-        uint32_t uiMax = reinterpret_cast<const uint32_t&>(_fMax);
+        auto uiMin = utility::binaryCopy<uint32_t>(_fMin);
+        auto uiMax = utility::binaryCopy<uint32_t>(_fMax);
 
         if (utility::isHexadecimal(strValue)) {
 
@@ -228,7 +227,7 @@ bool CFloatingPointParameterType::fromBlackboard(
     } else {
 
         // Move from "raw memory" value space to real space
-        float fValue = reinterpret_cast<const float&>(uiValue);
+        auto fValue = utility::binaryCopy<float>(uiValue);
 
         ostrStream << fValue;
     }
@@ -252,9 +251,7 @@ bool CFloatingPointParameterType::toBlackboard(
 
     // Cast is fine because dValue has been checked against the value range
     float fValue = static_cast<float>(dUserValue);
-    // Using an intermediary reference variable to avoid klocwork false positive
-    const uint32_t &value = reinterpret_cast<const uint32_t &>(fValue);
-    uiValue = value;
+    uiValue = utility::binaryCopy<decltype(uiValue)>(fValue);
     return true;
 }
 
@@ -264,8 +261,7 @@ bool CFloatingPointParameterType::fromBlackboard(
         CParameterAccessContext& /*ctx*/) const
 {
     // Move from "raw memory" value space to real space
-    // Using an intermediary reference variable to avoid klocwork false positive
-    const float &fValue = reinterpret_cast<const float &>(uiValue);
+    auto fValue = utility::binaryCopy<float>(uiValue);
 
     dUserValue = fValue;
     return true;

test/functional-tests/FloatingPoint.cpp

@@ -32,6 +32,7 @@
 #include "ParameterFramework.hpp"
 #include "ElementHandle.hpp"
 #include "Test.hpp"
+#include "BinaryCopy.hpp"
 
 #include <catch.hpp>
 
@@ -114,12 +115,12 @@ SCENARIO_METHOD(FloatsPF, "Floating points", "[floating points]") {
                 REQUIRE_NOTHROW(setRawValueSpace(true));
                 for (auto &vec : Tests<string>{
                             { "(too high, as decimal)",
-                                std::to_string(reinterpret_cast<const uint32_t&>(tooHigh)) },
+                                std::to_string(::utility::binaryCopy<uint32_t>(tooHigh)) },
                             { "(too low, as decimal)",
-                                std::to_string(reinterpret_cast<const uint32_t&>(tooLow)) },
+                                std::to_string(::utility::binaryCopy<uint32_t>(tooLow)) },
                             { "(meaningless)", "foobar" },
-                            { "(infinity)", std::to_string(reinterpret_cast<const uint32_t&>(inf))},
-                            { "(NaN)", std::to_string(reinterpret_cast<const uint32_t&>(nan))},
+                            { "(infinity)", std::to_string(::utility::binaryCopy<uint32_t>(inf))},
+                            { "(NaN)", std::to_string(::utility::binaryCopy<uint32_t>(nan))},
                         }) {
                     GIVEN("Invalid value " + vec.title) {
                         CHECK_THROWS_AS(setParameter(path, vec.payload), Exception);
@@ -130,11 +131,11 @@ SCENARIO_METHOD(FloatsPF, "Floating points", "[floating points]") {
                 const float zero = 0.0f;
                 for (auto &vec : Tests<string>{
                             { "(upper limit, as decimal)",
-                                std::to_string(reinterpret_cast<const uint32_t&>(upper)) },
+                                std::to_string(::utility::binaryCopy<uint32_t>(upper)) },
                             { "(lower limit, as decimal)",
-                                std::to_string(reinterpret_cast<const uint32_t&>(lower)) },
+                                std::to_string(::utility::binaryCopy<uint32_t>(lower)) },
                             { "(inside range, as decimal)",
-                                std::to_string(reinterpret_cast<const uint32_t&>(zero)) },
+                                std::to_string(::utility::binaryCopy<uint32_t>(zero)) },
                         }) {
                     GIVEN("A valid value " + vec.title) {
                         CHECK_NOTHROW(setParameter(path, vec.payload));

test/functional-tests/Handle.cpp

@@ -162,7 +162,10 @@ struct AllParamsPF : public ParameterFramework
             utility::TmpFile resultFile(result);
             utility::TmpFile expectedFile(expected);
             auto gitCommand = "git --no-pager diff --word-diff-regex='[^ <>]+' --color --no-index ";
-            system((gitCommand + resultFile.getPath() + ' ' + expectedFile.getPath()).c_str());
+            auto diffSuccess = system((gitCommand + resultFile.getPath() + ' ' + expectedFile.getPath()).c_str());
+            if (diffSuccess != 0) {
+                WARN("Failed to pretty-print the difference between actual and expected results.");
+            }
         }
     }
 

utility/BinaryCopy.hpp

@@ -0,0 +1,70 @@
+/*
+ * Copyright (c) 2015, Intel Corporation
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without modification,
+ * are permitted provided that the following conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright notice, this
+ * list of conditions and the following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above copyright notice,
+ * this list of conditions and the following disclaimer in the documentation and/or
+ * other materials provided with the distribution.
+ *
+ * 3. Neither the name of the copyright holder nor the names of its contributors
+ * may be used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+ * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR
+ * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
+ * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <algorithm>
+#include <type_traits>
+#include "Iterator.hpp"
+#include <cassert>
+
+namespace utility {
+
+/**
+ * Raw copy of one variable to another of the same size
+ *
+ * This can be regarder as a reinterpret_cast but does a copy and does not
+ * break strict-aliasing rules.
+ *
+ * The source and the destination must have the same storage size (e.g. copying
+ * a uint8_t into a uint32_t won't compile)
+ *
+ * @tparam Source The source type
+ * @tparam Destination the destination type (even if it is a reference, this
+ *         function returns by copy)
+ * @param source Source variable
+ * @returns the source, reinterpreted as the destination type
+ */
+template <class Destination, class Source>
+typename std::remove_reference<Destination>::type binaryCopy(const Source source)
+{
+    static_assert(sizeof(Source) == sizeof(Destination),
+                  "Source and Destination must have the same size");
+
+    using Destination_ = decltype(binaryCopy<Destination>(source));
+
+    union {
+        Source source;
+        Destination_ destination;
+    } hack;
+
+    hack.source = source;
+    return hack.destination;
+}
+
+} // namespace utility

utility/test/utility.cpp

@@ -29,6 +29,7 @@
  */
 
 #include "Utility.h"
+#include "BinaryCopy.hpp"
 
 #include <catch.hpp>
 #include <functional>
@@ -176,4 +177,52 @@ SCENARIO("isHexadecimal") {
     }
 }
 
+template <class T1, class T2>
+void checkBinaryEqual(T1 v1, T2 v2) {
+    // For some yet-unknown reason, GCC 4.8 complains about
+    //     CHECK(a == b);
+    // and suggests that parentheses should be added. This is related to catch
+    // internals but such construcuts have been used without problem in lots of
+    // other places...
+    // Besides, GCC 4.9 does not seem to have a problem with that either.
+    // As a workaround, captures variables and parenthesize the expressions.
+
+    auto v2AsT1 = utility::binaryCopy<T1>(v2);
+    CAPTURE(v1);
+    CAPTURE(v2AsT1);
+    CHECK((v1 == v2AsT1));
+
+    auto v1AsT2 = utility::binaryCopy<T2>(v1);
+    CAPTURE(v2);
+    CAPTURE(v1AsT2);
+    CHECK((v2 == v1AsT2));
+}
+
+SCENARIO("binaryCopy bit exactness") {
+    GIVEN("Integer representations computed using http://babbage.cs.qc.cuny.edu/IEEE-754/") {
+
+        THEN("Floats should be coded on 32bits and fulfill IEEE-754."
+             " That assumption is made in the Parameter Framework.") {
+            REQUIRE(sizeof(float) == sizeof(uint32_t));
+            REQUIRE(std::numeric_limits<float>::is_iec559);
+        }
+        WHEN("Testing float <=> uint32_t conversion") {
+            checkBinaryEqual<float, uint32_t>(1.23456f, 0x3f9e0610);
+        }
+
+        THEN("Doubles should be coded on 64bits and fulfill IEEE-754."
+             " That assumption is made in the Parameter Framework.") {
+            REQUIRE(sizeof(double) == sizeof(uint64_t));
+            REQUIRE(std::numeric_limits<double>::is_iec559);
+        }
+        WHEN("Testing double <=> uint64_t conversion") {
+            checkBinaryEqual<double, uint64_t>(987.65432109876, 0x408edd3c0cb3420e);
+        }
+    }
+
+    WHEN("Testing int8_t <=> uint8_t conversion") {
+        checkBinaryEqual<int8_t, uint8_t>(-1, 0xff);
+    }
+}
+
 } // namespace utility