Issue #10 - Fix non-standard c++ and some cleanup (#11)

improbable-eng · May 6, 2021 · 0d7d141 · 0d7d141
1 parent c6c25fe
commit 0d7d141
Show file tree

Hide file tree

Showing 15 changed files with 136 additions and 124 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ## [Unreleased]
 - Nothing yet.
 
+## [1.0.1] - 2021-05-06
+### Changed
+- replaced compilation flag `-fpermissive` with `-Werror`, and fixed all warnings/errors, see issue #10
 
 ## [1.0.0] - 2021-03-23
 ### Added
@@ -47,6 +50,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Nothing.
 
 
-[Unreleased]: https://github.com/improbable-eng/phtree-cpp/compare/v1.0.0...HEAD
+[Unreleased]: https://github.com/improbable-eng/phtree-cpp/compare/v1.0.1...HEAD
+[1.0.1]: https://github.com/improbable-eng/phtree-cpp/compare/v1.0.0...v1.0.1
 [1.0.0]: https://github.com/improbable-eng/phtree-cpp/compare/v0.1.0...v1.0.0
 [0.2.0]: https://github.com/improbable-eng/phtree-cpp/compare/v0.1.0...v0.2.0
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -1,7 +1,7 @@
 cmake_minimum_required(VERSION 3.14)
 
 # set the project name
-project(PH_Tree_Main VERSION 1.0.0
+project(PH_Tree_Main VERSION 1.0.1
         DESCRIPTION "PH-Tree C++"
         LANGUAGES CXX)
 
@@ -12,7 +12,7 @@ endif()
 # specify the C++ standard
 set(CMAKE_CXX_STANDARD 17)
 set(CMAKE_CXX_STANDARD_REQUIRED True)
-set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++17 -fpermissive")
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++17 -Wall -Werror")
 set(CMAKE_CXX_FLAGS_RELEASE "-O3")
 
 add_subdirectory(phtree)

diff --git a/README.md b/README.md
@@ -547,7 +547,6 @@ cmake --build .
 
 The PH-Tree is discussed in the follwoing publications and reports:   
 
-*
 - T. Zaeschke, C. Zimmerli, M.C. Norrie:
   "The PH-Tree -- A Space-Efficient Storage Structure and Multi-Dimensional Index", (SIGMOD 2014)
 - T. Zaeschke: "The PH-Tree Revisited", (2015)

diff --git a/phtree/common/converter.h b/phtree/common/converter.h
@@ -18,6 +18,7 @@
 #define PHTREE_COMMON_CONVERTER_H
 
 #include "base_types.h"
+#include <cstring>
 
 /*
  * PLEASE do not include this file directly, it is included via common.h.
@@ -37,13 +38,18 @@ class ScalarConverterIEEE {
         // This result is properly ordered longs for all positive doubles. Negative values have
         // inverse ordering. For negative doubles, we therefore simply invert them to make them
         // sortable, however the sign must be inverted again to stay negative.
-        scalar_64_t r = reinterpret_cast<scalar_64_t&>(value);
+        // Also, we could use reinterpret_cast, but that fails on GCC. Using memcpy results in the
+        // same asm instructions as reinterpret_cast().
+        scalar_64_t r;
+        memcpy(&r, &value, sizeof(r));
         return r >= 0 ? r : r ^ 0x7FFFFFFFFFFFFFFFL;
     }
 
     static double post(scalar_64_t value) {
         auto v = value >= 0 ? value : value ^ 0x7FFFFFFFFFFFFFFFL;
-        return reinterpret_cast<double&>(v);
+        double r;
+        memcpy(&r, &v, sizeof(r));
+        return r;
     }
 
     static scalar_32_t pre(float value) {
@@ -52,13 +58,16 @@ class ScalarConverterIEEE {
         // This result is properly ordered longs for all positive doubles. Negative values have
         // inverse ordering. For negative doubles, we therefore simply invert them to make them
         // sortable, however the sign must be inverted again to stay negative.
-        scalar_32_t r = reinterpret_cast<scalar_32_t&>(value);
+        scalar_32_t r;
+        memcpy(&r, &value, sizeof(r));
         return r >= 0 ? r : r ^ 0x7FFFFFFFL;
     }
 
     static float post(scalar_32_t value) {
         auto v = value >= 0 ? value : value ^ 0x7FFFFFFFL;
-        return reinterpret_cast<float&>(v);
+        float r;
+        memcpy(&r, &v, sizeof(r));
+        return r;
     }
 };
 

diff --git a/phtree/v16/debug_helper_v16.h b/phtree/v16/debug_helper_v16.h
@@ -17,9 +17,9 @@
 #ifndef PHTREE_V16_DEBUG_HELPER_H
 #define PHTREE_V16_DEBUG_HELPER_H
 
-#include "node.h"
 #include "../common/common.h"
 #include "../common/debug_helper.h"
+#include "node.h"
 #include "phtree_v16.h"
 #include <string>
 
@@ -30,12 +30,11 @@ class PhTreeV16;
 
 template <dimension_t DIM, typename T, typename SCALAR>
 class DebugHelperV16 : public PhTreeDebugHelper::DebugHelper {
-    using Key = PhPoint<DIM, SCALAR>;
-    using Node = Node<DIM, T, SCALAR>;
-    using Entry = Entry<DIM, T, SCALAR>;
+    using KeyT = PhPoint<DIM, SCALAR>;
+    using NodeT = Node<DIM, T, SCALAR>;
 
   public:
-    DebugHelperV16(const Node& root, size_t size) : root_{root}, size_{size} {}
+    DebugHelperV16(const NodeT& root, size_t size) : root_{root}, size_{size} {}
 
     /*
      * Depending on the detail parameter this returns:
@@ -58,7 +57,7 @@ class DebugHelperV16 : public PhTreeDebugHelper::DebugHelper {
             ToStringPlain(os, root_);
             break;
         case Enum::tree:
-            ToStringTree(os, 0, root_, Key(), true);
+            ToStringTree(os, 0, root_, KeyT{}, true);
             break;
         }
         return os.str();
@@ -83,9 +82,9 @@ class DebugHelperV16 : public PhTreeDebugHelper::DebugHelper {
     }
 
   private:
-    void ToStringPlain(std::ostringstream& os, const Node& node) const {
+    void ToStringPlain(std::ostringstream& os, const NodeT& node) const {
         for (auto& it : node.Entries()) {
-            const Entry& o = it.second;
+            const auto& o = it.second;
             // inner node?
             if (o.IsNode()) {
                 ToStringPlain(os, o.GetNode());
@@ -99,8 +98,8 @@ class DebugHelperV16 : public PhTreeDebugHelper::DebugHelper {
     void ToStringTree(
         std::ostringstream& sb,
         bit_width_t current_depth,
-        const Node& node,
-        const Key& prefix,
+        const NodeT& node,
+        const KeyT& prefix,
         bool printValue) const {
         std::string ind = "*";
         for (bit_width_t i = 0; i < current_depth; ++i) {
@@ -142,7 +141,7 @@ class DebugHelperV16 : public PhTreeDebugHelper::DebugHelper {
         }
     }
 
-    const Node& root_;
+    const NodeT& root_;
     const size_t size_;
 };
 }  // namespace improbable::phtree::v16

diff --git a/phtree/v16/entry.h b/phtree/v16/entry.h
@@ -17,8 +17,8 @@
 #ifndef PHTREE_V16_ENTRY_H
 #define PHTREE_V16_ENTRY_H
 
-#include "node.h"
 #include "../../phtree/common/common.h"
+#include "node.h"
 #include <cassert>
 #include <cstdint>
 #include <memory>
@@ -39,56 +39,57 @@ class Node;
  */
 template <dimension_t DIM, typename T, typename SCALAR>
 class Entry {
-    using Key = PhPoint<DIM, SCALAR>;
-    using Value = std::remove_const_t<T>;
-    using Node = Node<DIM, T, SCALAR>;
+    using KeyT = PhPoint<DIM, SCALAR>;
+    using ValueT = std::remove_const_t<T>;
+    using NodeT = Node<DIM, T, SCALAR>;
 
   public:
-    Entry() : kd_key_(), value_{std::in_place_type<Value>, T{}} {}
+    Entry() : kd_key_(), value_{std::in_place_type<ValueT>, T{}} {}
 
     /*
      * Construct entry with existing node.
      */
-    Entry(const Key& k, std::unique_ptr<Node>&& node)
+    Entry(const KeyT& k, std::unique_ptr<NodeT>&& node)
     : kd_key_{k}
-    , value_{std::in_place_type<std::unique_ptr<Node>>, std::forward<std::unique_ptr<Node>>(node)} {
-    }
+    , value_{
+          std::in_place_type<std::unique_ptr<NodeT>>, std::forward<std::unique_ptr<NodeT>>(node)} {}
 
     /*
      * Construct entry with a new node.
      */
     Entry(bit_width_t infix_len, bit_width_t postfix_len)
     : kd_key_()
-    , value_{std::in_place_type<std::unique_ptr<Node>>,
-             std::make_unique<Node>(infix_len, postfix_len)} {}
+    , value_{
+          std::in_place_type<std::unique_ptr<NodeT>>,
+          std::make_unique<NodeT>(infix_len, postfix_len)} {}
 
     /*
      * Construct entry with new T or moved T.
      */
     template <typename... _Args>
-    explicit Entry(const Key& k, _Args&&... __args)
-    : kd_key_{k}, value_{std::in_place_type<Value>, std::forward<_Args>(__args)...} {}
+    explicit Entry(const KeyT& k, _Args&&... __args)
+    : kd_key_{k}, value_{std::in_place_type<ValueT>, std::forward<_Args>(__args)...} {}
 
-    [[nodiscard]] const Key& GetKey() const {
+    [[nodiscard]] const KeyT& GetKey() const {
         return kd_key_;
     }
 
     [[nodiscard]] bool IsValue() const {
-        return std::holds_alternative<Value>(value_);
+        return std::holds_alternative<ValueT>(value_);
     }
 
     [[nodiscard]] bool IsNode() const {
-        return std::holds_alternative<std::unique_ptr<Node>>(value_);
+        return std::holds_alternative<std::unique_ptr<NodeT>>(value_);
     }
 
     [[nodiscard]] T& GetValue() const {
         assert(IsValue());
-        return const_cast<T&>(std::get<Value>(value_));
+        return const_cast<T&>(std::get<ValueT>(value_));
     }
 
-    [[nodiscard]] Node& GetNode() const {
+    [[nodiscard]] NodeT& GetNode() const {
         assert(IsNode());
-        return *std::get<std::unique_ptr<Node>>(value_);
+        return *std::get<std::unique_ptr<NodeT>>(value_);
     }
 
     void ReplaceNodeWithDataFromEntry(Entry&& other) {
@@ -98,14 +99,14 @@ class Entry {
         // 'value_' points indirectly to 'entry' so we have to remove `entity's` content before
         // assigning anything to `value_` here. Otherwise the assignment would destruct the previous
         // content and, by reachability, `entity's` content.
-        auto old_node = std::get<std::unique_ptr<Node>>(value_).release();
+        auto old_node = std::get<std::unique_ptr<NodeT>>(value_).release();
         value_ = std::move(other.value_);
         delete old_node;
     }
 
   private:
-    Key kd_key_;
-    std::variant<Value, std::unique_ptr<Node>> value_;
+    KeyT kd_key_;
+    std::variant<ValueT, std::unique_ptr<NodeT>> value_;
 };
 }  // namespace improbable::phtree::v16
 

diff --git a/phtree/v16/for_each.h b/phtree/v16/for_each.h
@@ -17,8 +17,8 @@
 #ifndef PHTREE_V16_FOR_EACH_H
 #define PHTREE_V16_FOR_EACH_H
 
-#include "iterator_simple.h"
 #include "../common/common.h"
+#include "iterator_simple.h"
 
 namespace improbable::phtree::v16 {
 
@@ -32,20 +32,20 @@ class ForEach {
     using KeyExternal = typename CONVERT::KeyExternal;
     using KeyInternal = typename CONVERT::KeyInternal;
     using SCALAR = typename CONVERT::ScalarInternal;
-    using Entry = Entry<DIM, T, SCALAR>;
-    using Node = Node<DIM, T, SCALAR>;
+    using EntryT = Entry<DIM, T, SCALAR>;
+    using NodeT = Node<DIM, T, SCALAR>;
 
   public:
     ForEach(const CONVERT& converter, CALLBACK_FN& callback, FILTER filter)
     : converter_{converter}, callback_{callback}, filter_(std::move(filter)) {}
 
-    void run(const Entry& root) {
+    void run(const EntryT& root) {
         assert(root.IsNode());
         TraverseNode(root.GetKey(), root.GetNode());
     }
 
   private:
-    void TraverseNode(const KeyInternal& key, const Node& node) {
+    void TraverseNode(const KeyInternal& key, const NodeT& node) {
         auto iter = node.Entries().begin();
         auto end = node.Entries().end();
         for (; iter != end; ++iter) {

diff --git a/phtree/v16/for_each_hc.h b/phtree/v16/for_each_hc.h
@@ -17,8 +17,8 @@
 #ifndef PHTREE_V16_FOR_EACH_HC_H
 #define PHTREE_V16_FOR_EACH_HC_H
 
-#include "iterator_simple.h"
 #include "../common/common.h"
+#include "iterator_simple.h"
 
 namespace improbable::phtree::v16 {
 
@@ -39,8 +39,8 @@ class ForEachHC {
     using KeyExternal = typename CONVERT::KeyExternal;
     using KeyInternal = typename CONVERT::KeyInternal;
     using SCALAR = typename CONVERT::ScalarInternal;
-    using Entry = Entry<DIM, T, SCALAR>;
-    using Node = Node<DIM, T, SCALAR>;
+    using EntryT = Entry<DIM, T, SCALAR>;
+    using NodeT = Node<DIM, T, SCALAR>;
 
   public:
     ForEachHC(
@@ -55,13 +55,13 @@ class ForEachHC {
     , callback_{callback}
     , filter_(std::move(filter)) {}
 
-    void run(const Entry& root) {
+    void run(const EntryT& root) {
         assert(root.IsNode());
         TraverseNode(root.GetKey(), root.GetNode());
     }
 
   private:
-    void TraverseNode(const KeyInternal& key, const Node& node) {
+    void TraverseNode(const KeyInternal& key, const NodeT& node) {
         hc_pos_t mask_lower = 0;
         hc_pos_t mask_upper = 0;
         CalcLimits(node.GetPostfixLen(), key, mask_lower, mask_upper);
@@ -90,7 +90,7 @@ class ForEachHC {
         }
     }
 
-    bool CheckNode(const KeyInternal& key, const Node& node) const {
+    bool CheckNode(const KeyInternal& key, const NodeT& node) const {
         // Check if the node overlaps with the query box.
         // An infix with len=0 implies that at least part of the child node overlaps with the query,
         // otherwise the bit mask checking would have returned 'false'.
@@ -108,7 +108,7 @@ class ForEachHC {
         return ApplyFilter(key, node);
     }
 
-    [[nodiscard]] bool ApplyFilter(const KeyInternal& key, const Node& node) const {
+    [[nodiscard]] bool ApplyFilter(const KeyInternal& key, const NodeT& node) const {
         return filter_.IsNodeValid(key, node.GetPostfixLen() + 1);
     }