Skip to content

Commit

Permalink
Takes care of proper special member generation globally, fixes #1689
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
daniel-j-h committed Jan 27, 2016
1 parent 0802804 commit 2d79f90
Show file tree
Hide file tree
Showing 12 changed files with 55 additions and 43 deletions.
3 changes: 3 additions & 0 deletions include/engine/engine.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
18 changes: 11 additions & 7 deletions include/engine/routing_algorithms/routing_base.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,12 @@ template <class DataFacadeT, class Derived> 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.
Expand Down Expand Up @@ -98,7 +99,8 @@ template <class DataFacadeT, class Derived> 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;
Expand Down Expand Up @@ -362,8 +364,10 @@ template <class DataFacadeT, class Derived> 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});
}
}
Expand Down Expand Up @@ -601,8 +605,8 @@ template <class DataFacadeT, class Derived> class BasicRoutingInterface

// TODO check if unordered_set might be faster
// sort by id and increasing by distance
auto entry_point_comparator = [](const std::pair<NodeID, EdgeWeight> &lhs,
const std::pair<NodeID, EdgeWeight> &rhs)
auto entry_point_comparator =
[](const std::pair<NodeID, EdgeWeight> &lhs, const std::pair<NodeID, EdgeWeight> &rhs)
{
return lhs.first < rhs.first || (lhs.first == rhs.first && lhs.second < rhs.second);
};
Expand Down
4 changes: 2 additions & 2 deletions include/extractor/edge_based_graph_factory.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<util::NodeBasedDynamicGraph> node_based_graph,
const CompressedEdgeContainer &compressed_edge_container,
Expand All @@ -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<EdgeBasedEdge> &edges);
void GetEdgeBasedNodes(std::vector<EdgeBasedNode> &nodes);
void GetStartPointMarkers(std::vector<bool> &node_is_startpoint);
Expand Down
5 changes: 3 additions & 2 deletions include/extractor/extractor_callbacks.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 &current_node, const ExtractionNode &result_node);

Expand Down
4 changes: 3 additions & 1 deletion include/extractor/scripting_environment.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion include/server/connection.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class Connection : public std::enable_shared_from_this<Connection>
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();

Expand Down
1 change: 1 addition & 0 deletions include/server/request_handler.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ class RequestHandler

RequestHandler();
RequestHandler(const RequestHandler &) = delete;
RequestHandler &operator=(const RequestHandler &) = delete;

void handle_request(const http::request &current_request, http::reply &current_reply);
void RegisterRoutingMachine(OSRM *osrm);
Expand Down
56 changes: 29 additions & 27 deletions include/storage/shared_memory.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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;

Expand Down Expand Up @@ -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;

Expand All @@ -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)
Expand Down Expand Up @@ -324,34 +327,33 @@ class SharedMemory

template <typename IdentifierT, typename LockFileT = OSRMLockFile>
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());
}
}

}
}

Expand Down
1 change: 1 addition & 0 deletions include/util/simple_logger.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class LogPolicy
static LogPolicy &GetInstance();

LogPolicy(const LogPolicy &) = delete;
LogPolicy &operator=(const LogPolicy &) = delete;

private:
LogPolicy() : m_is_mute(true) {}
Expand Down
2 changes: 1 addition & 1 deletion include/util/static_rtree.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,8 @@ class StaticRTree
boost::filesystem::ifstream leaves_stream;

public:
StaticRTree() = delete;
StaticRTree(const StaticRTree &) = delete;
StaticRTree &operator=(const StaticRTree &) = delete;

template <typename CoordinateT>
// Construct a packed Hilbert-R-Tree with Kamel-Faloutsos algorithm [1]
Expand Down
1 change: 0 additions & 1 deletion src/engine/douglas_peucker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ namespace
{
struct CoordinatePairCalculator
{
CoordinatePairCalculator() = delete;
CoordinatePairCalculator(const util::FixedPointCoordinate coordinate_a,
const util::FixedPointCoordinate coordinate_b)
{
Expand Down
1 change: 0 additions & 1 deletion src/server/request_handler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ void RequestHandler::handle_request(const http::request &current_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;
}
Expand Down

0 comments on commit 2d79f90

Please sign in to comment.