From 2d79f90f413326683e8712b278610208194fde90 Mon Sep 17 00:00:00 2001 From: "Daniel J. Hofmann" Date: Wed, 27 Jan 2016 11:20:55 +0100 Subject: [PATCH] Takes care of proper special member generation globally, fixes #1689 Phew, a lot of classes were affected by this. The rationale for the changes are as follows: - When a type X declares any constructor, the default constructor is not declared, so there is no need for X() = delete there. In fact, there is brutal difference between those two: deleted members participate in overload resolution, but not-declared members do not! - When a type X wants to be non-copyable (e.g. to be only movable, like threads, unique_ptrs, and so on), you can either do it by inheriting from boost::noncopyable (the old way), or better declare both (!) the copy constructor _and_ the copy assignment operator as deleted: X(X const&) = delete; X& operator=(X const&) = delete; We had tons of types with deleted copy constructors that were lacking a corresponding deleted copy assignment operator, making them still copyable and you wouldn't even notice (read: scary)! References: - http://accu.org/content/conf2014/Howard_Hinnant_Accu_2014.pdf - http://www.boost.org/doc/libs/master/libs/core/doc/html/core/noncopyable.html Note: I know, I'm quoting Hinnant's extraordinary slides a lot, but getting the sematic right here is so incredibly important. --- include/engine/engine.hpp | 3 + .../routing_algorithms/routing_base.hpp | 18 +++--- .../extractor/edge_based_graph_factory.hpp | 4 +- include/extractor/extractor_callbacks.hpp | 5 +- include/extractor/scripting_environment.hpp | 4 +- include/server/connection.hpp | 2 +- include/server/request_handler.hpp | 1 + include/storage/shared_memory.hpp | 56 ++++++++++--------- include/util/simple_logger.hpp | 1 + include/util/static_rtree.hpp | 2 +- src/engine/douglas_peucker.cpp | 1 - src/server/request_handler.cpp | 1 - 12 files changed, 55 insertions(+), 43 deletions(-) diff --git a/include/engine/engine.hpp b/include/engine/engine.hpp index 3b23445ba37..1ca8ad60908 100644 --- a/include/engine/engine.hpp +++ b/include/engine/engine.hpp @@ -46,7 +46,10 @@ class Engine final public: Engine(EngineConfig &config_); + Engine(const Engine &) = delete; + Engine &operator=(const Engine &) = delete; + int RunQuery(const RouteParameters &route_parameters, util::json::Object &json_result); private: diff --git a/include/engine/routing_algorithms/routing_base.hpp b/include/engine/routing_algorithms/routing_base.hpp index 6f9315d1e9d..156ebba9e0a 100644 --- a/include/engine/routing_algorithms/routing_base.hpp +++ b/include/engine/routing_algorithms/routing_base.hpp @@ -42,11 +42,12 @@ template class BasicRoutingInterface DataFacadeT *facade; public: - BasicRoutingInterface() = delete; - BasicRoutingInterface(const BasicRoutingInterface &) = delete; explicit BasicRoutingInterface(DataFacadeT *facade) : facade(facade) {} ~BasicRoutingInterface() {} + BasicRoutingInterface(const BasicRoutingInterface &) = delete; + BasicRoutingInterface &operator=(const BasicRoutingInterface &) = delete; + /* min_edge_offset is needed in case we use multiple nodes as start/target nodes with different (even negative) offsets. @@ -98,7 +99,8 @@ template class BasicRoutingInterface (!force_loop_forward || forward_heap.GetData(node).parent != node) // if loops are forced, they are so at the source - && (!force_loop_reverse || reverse_heap.GetData(node).parent != node)) + && + (!force_loop_reverse || reverse_heap.GetData(node).parent != node)) { middle_node_id = node; upper_bound = new_distance; @@ -362,8 +364,10 @@ template class BasicRoutingInterface BOOST_ASSERT(i < id_vector.size()); BOOST_ASSERT(phantom_node_pair.target_phantom.forward_travel_mode > 0); unpacked_path.emplace_back( - PathData{id_vector[i], phantom_node_pair.target_phantom.name_id, - extractor::TurnInstruction::NoTurn, 0, + PathData{id_vector[i], + phantom_node_pair.target_phantom.name_id, + extractor::TurnInstruction::NoTurn, + 0, phantom_node_pair.target_phantom.forward_travel_mode}); } } @@ -601,8 +605,8 @@ template class BasicRoutingInterface // TODO check if unordered_set might be faster // sort by id and increasing by distance - auto entry_point_comparator = [](const std::pair &lhs, - const std::pair &rhs) + auto entry_point_comparator = + [](const std::pair &lhs, const std::pair &rhs) { return lhs.first < rhs.first || (lhs.first == rhs.first && lhs.second < rhs.second); }; diff --git a/include/extractor/edge_based_graph_factory.hpp b/include/extractor/edge_based_graph_factory.hpp index ef9bacf99cd..7957cb60be1 100644 --- a/include/extractor/edge_based_graph_factory.hpp +++ b/include/extractor/edge_based_graph_factory.hpp @@ -36,8 +36,8 @@ namespace extractor class EdgeBasedGraphFactory { public: - EdgeBasedGraphFactory() = delete; EdgeBasedGraphFactory(const EdgeBasedGraphFactory &) = delete; + EdgeBasedGraphFactory &operator=(const EdgeBasedGraphFactory &) = delete; explicit EdgeBasedGraphFactory(std::shared_ptr node_based_graph, const CompressedEdgeContainer &compressed_edge_container, @@ -62,7 +62,7 @@ class EdgeBasedGraphFactory const bool generate_edge_lookup); #endif - //The following get access functions destroy the content in the factory + // The following get access functions destroy the content in the factory void GetEdgeBasedEdges(util::DeallocatingVector &edges); void GetEdgeBasedNodes(std::vector &nodes); void GetStartPointMarkers(std::vector &node_is_startpoint); diff --git a/include/extractor/extractor_callbacks.hpp b/include/extractor/extractor_callbacks.hpp index 673c2f60139..db4e870f3e1 100644 --- a/include/extractor/extractor_callbacks.hpp +++ b/include/extractor/extractor_callbacks.hpp @@ -38,10 +38,11 @@ class ExtractorCallbacks ExtractionContainers &external_memory; public: - ExtractorCallbacks() = delete; - ExtractorCallbacks(const ExtractorCallbacks &) = delete; explicit ExtractorCallbacks(ExtractionContainers &extraction_containers); + ExtractorCallbacks(const ExtractorCallbacks &) = delete; + ExtractorCallbacks &operator=(const ExtractorCallbacks &) = delete; + // warning: caller needs to take care of synchronization! void ProcessNode(const osmium::Node ¤t_node, const ExtractionNode &result_node); diff --git a/include/extractor/scripting_environment.hpp b/include/extractor/scripting_environment.hpp index d02c7cb264d..cd9de04ccf4 100644 --- a/include/extractor/scripting_environment.hpp +++ b/include/extractor/scripting_environment.hpp @@ -23,9 +23,11 @@ namespace extractor class ScriptingEnvironment { public: - ScriptingEnvironment() = delete; explicit ScriptingEnvironment(const std::string &file_name); + ScriptingEnvironment(const ScriptingEnvironment &) = delete; + ScriptingEnvironment &operator=(const ScriptingEnvironment &) = delete; + lua_State *GetLuaState(); private: diff --git a/include/server/connection.hpp b/include/server/connection.hpp index ace005392bd..08821597e28 100644 --- a/include/server/connection.hpp +++ b/include/server/connection.hpp @@ -39,7 +39,7 @@ class Connection : public std::enable_shared_from_this public: explicit Connection(boost::asio::io_service &io_service, RequestHandler &handler); Connection(const Connection &) = delete; - Connection() = delete; + Connection &operator=(const Connection &) = delete; boost::asio::ip::tcp::socket &socket(); diff --git a/include/server/request_handler.hpp b/include/server/request_handler.hpp index eba77eac721..16b3886a6e6 100644 --- a/include/server/request_handler.hpp +++ b/include/server/request_handler.hpp @@ -28,6 +28,7 @@ class RequestHandler RequestHandler(); RequestHandler(const RequestHandler &) = delete; + RequestHandler &operator=(const RequestHandler &) = delete; void handle_request(const http::request ¤t_request, http::reply ¤t_reply); void RegisterRoutingMachine(OSRM *osrm); diff --git a/include/storage/shared_memory.hpp b/include/storage/shared_memory.hpp index 3b04d2e32a8..7f14482e37d 100644 --- a/include/storage/shared_memory.hpp +++ b/include/storage/shared_memory.hpp @@ -58,7 +58,10 @@ class SharedMemory } shm_remove() : m_shmid(INT_MIN), m_initialized(false) {} + shm_remove(const shm_remove &) = delete; + shm_remove &operator=(const shm_remove &) = delete; + ~shm_remove() { if (m_initialized) @@ -75,7 +78,6 @@ class SharedMemory public: void *Ptr() const { return region.get_address(); } - SharedMemory() = delete; SharedMemory(const SharedMemory &) = delete; SharedMemory &operator=(const SharedMemory &) = delete; @@ -193,8 +195,6 @@ class SharedMemory class shm_remove { private: - shm_remove(const shm_remove &) = delete; - shm_remove &operator=(const shm_remove &) = delete; char *m_shmid; bool m_initialized; @@ -207,6 +207,9 @@ class SharedMemory shm_remove() : m_shmid("undefined"), m_initialized(false) {} + shm_remove(const shm_remove &) = delete; + shm_remove &operator=(const shm_remove &) = delete; + ~shm_remove() { if (m_initialized) @@ -324,34 +327,33 @@ class SharedMemory template SharedMemory *makeSharedMemory(const IdentifierT &id, - const uint64_t size = 0, - bool read_write = false, - bool remove_prev = true) + const uint64_t size = 0, + bool read_write = false, + bool remove_prev = true) { - try - { - LockFileT lock_file; - if (!boost::filesystem::exists(lock_file())) + try { - if (0 == size) - { - throw util::exception("lock file does not exist, exiting"); - } - else - { - boost::filesystem::ofstream ofs(lock_file()); - } + LockFileT lock_file; + if (!boost::filesystem::exists(lock_file())) + { + if (0 == size) + { + throw util::exception("lock file does not exist, exiting"); + } + else + { + boost::filesystem::ofstream ofs(lock_file()); + } + } + return new SharedMemory(lock_file(), id, size, read_write, remove_prev); + } + catch (const boost::interprocess::interprocess_exception &e) + { + util::SimpleLogger().Write(logWARNING) << "caught exception: " << e.what() << ", code " + << e.get_error_code(); + throw util::exception(e.what()); } - return new SharedMemory(lock_file(), id, size, read_write, remove_prev); - } - catch (const boost::interprocess::interprocess_exception &e) - { - util::SimpleLogger().Write(logWARNING) << "caught exception: " << e.what() << ", code " - << e.get_error_code(); - throw util::exception(e.what()); - } } - } } diff --git a/include/util/simple_logger.hpp b/include/util/simple_logger.hpp index 29301b9d591..f71f0ff7de1 100644 --- a/include/util/simple_logger.hpp +++ b/include/util/simple_logger.hpp @@ -29,6 +29,7 @@ class LogPolicy static LogPolicy &GetInstance(); LogPolicy(const LogPolicy &) = delete; + LogPolicy &operator=(const LogPolicy &) = delete; private: LogPolicy() : m_is_mute(true) {} diff --git a/include/util/static_rtree.hpp b/include/util/static_rtree.hpp index 32bc71eee01..c6f34f71dce 100644 --- a/include/util/static_rtree.hpp +++ b/include/util/static_rtree.hpp @@ -106,8 +106,8 @@ class StaticRTree boost::filesystem::ifstream leaves_stream; public: - StaticRTree() = delete; StaticRTree(const StaticRTree &) = delete; + StaticRTree &operator=(const StaticRTree &) = delete; template // Construct a packed Hilbert-R-Tree with Kamel-Faloutsos algorithm [1] diff --git a/src/engine/douglas_peucker.cpp b/src/engine/douglas_peucker.cpp index be3572306b2..2685b4a2a7c 100644 --- a/src/engine/douglas_peucker.cpp +++ b/src/engine/douglas_peucker.cpp @@ -19,7 +19,6 @@ namespace { struct CoordinatePairCalculator { - CoordinatePairCalculator() = delete; CoordinatePairCalculator(const util::FixedPointCoordinate coordinate_a, const util::FixedPointCoordinate coordinate_b) { diff --git a/src/server/request_handler.cpp b/src/server/request_handler.cpp index 96b94671da1..6df6c1e99f0 100644 --- a/src/server/request_handler.cpp +++ b/src/server/request_handler.cpp @@ -147,7 +147,6 @@ void RequestHandler::handle_request(const http::request ¤t_request, catch (const std::exception &e) { current_reply = http::reply::stock_reply(http::reply::internal_server_error); - ; util::SimpleLogger().Write(logWARNING) << "[server error] code: " << e.what() << ", uri: " << current_request.uri; }