Refactor creation/cloning of output_t and derived classes

These were a bit of a mess. Now the implementations of the derived
classes are more similar and a bit simpler.

We'll use the "rule of five" in output_t and derived classes, so copy
and move constructors and operators as well as (virtual) destructor
are all spelled out.

As far as possible constructors etc. are made private/protected. We only
ever create those classes though the create_output() function or the
clone() function. But we are using std::make_shared() internally so some
constructors need to be public because, while conceptually the objects
are created from inside the class itself, it is the make_shared function
that actually does this and it can only use the public interface.

Author: joto

src/output-flex.cpp

@@ -1980,45 +1980,50 @@ void output_flex_t::start()
     }
 }
 
+output_flex_t::output_flex_t(output_flex_t const *other,
+                             std::shared_ptr<middle_query_t> mid,
+                             std::shared_ptr<db_copy_thread_t> copy_thread)
+: output_t(other, std::move(mid)), m_tables(other->m_tables),
+  m_stage2_way_ids(other->m_stage2_way_ids),
+  m_copy_thread(std::move(copy_thread)), m_lua_state(other->m_lua_state),
+  m_expire(other->get_options()->expire_tiles_zoom,
+           other->get_options()->expire_tiles_max_bbox,
+           other->get_options()->projection),
+  m_process_node(other->m_process_node), m_process_way(other->m_process_way),
+  m_process_relation(other->m_process_relation),
+  m_select_relation_members(other->m_select_relation_members)
+{
+    assert(m_table_connections.empty());
+    for (auto &table : *m_tables) {
+        m_table_connections.emplace_back(&table, m_copy_thread);
+    }
+
+    init_clone();
+}
+
 std::shared_ptr<output_t>
 output_flex_t::clone(std::shared_ptr<middle_query_t> const &mid,
                      std::shared_ptr<db_copy_thread_t> const &copy_thread) const
 {
-    return std::make_shared<output_flex_t>(
-        mid, m_thread_pool, *get_options(), copy_thread, true, m_lua_state,
-        m_process_node, m_process_way, m_process_relation,
-        m_select_relation_members, m_tables, m_stage2_way_ids);
+    return std::make_shared<output_flex_t>(this, mid, copy_thread);
 }
 
 output_flex_t::output_flex_t(
     std::shared_ptr<middle_query_t> const &mid,
     std::shared_ptr<thread_pool_t> thread_pool, options_t const &o,
-    std::shared_ptr<db_copy_thread_t> const &copy_thread, bool is_clone,
-    std::shared_ptr<lua_State> lua_state, prepared_lua_function_t process_node,
-    prepared_lua_function_t process_way,
-    prepared_lua_function_t process_relation,
-    prepared_lua_function_t select_relation_members,
-    std::shared_ptr<std::vector<flex_table_t>> tables,
-    std::shared_ptr<idset_t> stage2_way_ids)
-: output_t(mid, std::move(thread_pool), o), m_tables(std::move(tables)),
-  m_stage2_way_ids(std::move(stage2_way_ids)), m_copy_thread(copy_thread),
-  m_lua_state(std::move(lua_state)),
-  m_expire(o.expire_tiles_zoom, o.expire_tiles_max_bbox, o.projection),
-  m_process_node(process_node), m_process_way(process_way),
-  m_process_relation(process_relation),
-  m_select_relation_members(select_relation_members)
+    std::shared_ptr<db_copy_thread_t> const &copy_thread)
+: output_t(mid, std::move(thread_pool), o), m_copy_thread(copy_thread),
+  m_expire(o.expire_tiles_zoom, o.expire_tiles_max_bbox, o.projection)
 {
     assert(copy_thread);
 
-    if (!is_clone) {
-        init_lua(get_options()->style);
+    init_lua(get_options()->style);
 
-        // If the osm2pgsql.select_relation_members() Lua function is defined
-        // it means we need two-stage processing which in turn means we need
-        // the full ways stored in the middle.
-        if (m_select_relation_members) {
-            m_output_requirements.full_ways = true;
-        }
+    // If the osm2pgsql.select_relation_members() Lua function is defined
+    // it means we need two-stage processing which in turn means we need
+    // the full ways stored in the middle.
+    if (m_select_relation_members) {
+        m_output_requirements.full_ways = true;
     }
 
     if (m_tables->empty()) {
@@ -2030,10 +2035,6 @@ output_flex_t::output_flex_t(
     for (auto &table : *m_tables) {
         m_table_connections.emplace_back(&table, m_copy_thread);
     }
-
-    if (is_clone) {
-        init_clone();
-    }
 }
 
 /**

src/output-flex.hpp

@@ -97,20 +97,17 @@ class prepared_lua_function_t
 
 class output_flex_t : public output_t
 {
-
 public:
+    /// Constructor for new objects
     output_flex_t(
         std::shared_ptr<middle_query_t> const &mid,
         std::shared_ptr<thread_pool_t> thread_pool, options_t const &options,
-        std::shared_ptr<db_copy_thread_t> const &copy_thread,
-        bool is_clone = false, std::shared_ptr<lua_State> lua_state = nullptr,
-        prepared_lua_function_t process_node = {},
-        prepared_lua_function_t process_way = {},
-        prepared_lua_function_t process_relation = {},
-        prepared_lua_function_t select_relation_members = {},
-        std::shared_ptr<std::vector<flex_table_t>> tables =
-            std::make_shared<std::vector<flex_table_t>>(),
-        std::shared_ptr<idset_t> stage2_way_ids = std::make_shared<idset_t>());
+        std::shared_ptr<db_copy_thread_t> const &copy_thread);
+
+    /// Constructor for cloned objects
+    output_flex_t(output_flex_t const *other,
+                  std::shared_ptr<middle_query_t> mid,
+                  std::shared_ptr<db_copy_thread_t> copy_thread);
 
     output_flex_t(output_flex_t const &) = delete;
     output_flex_t &operator=(output_flex_t const &) = delete;
@@ -281,12 +278,14 @@ class output_flex_t : public output_t
 
     }; // relation_cache_t
 
-    std::shared_ptr<std::vector<flex_table_t>> m_tables;
+    std::shared_ptr<std::vector<flex_table_t>> m_tables =
+        std::make_shared<std::vector<flex_table_t>>();
+
     std::vector<table_connection_t> m_table_connections;
 
     // This is shared between all clones of the output and must only be
     // accessed while protected using the lua_mutex.
-    std::shared_ptr<idset_t> m_stage2_way_ids;
+    std::shared_ptr<idset_t> m_stage2_way_ids = std::make_shared<idset_t>();
 
     std::shared_ptr<db_copy_thread_t> m_copy_thread;
 
@@ -300,10 +299,10 @@ class output_flex_t : public output_t
     relation_cache_t m_relation_cache;
     osmium::Node const *m_context_node = nullptr;
 
-    prepared_lua_function_t m_process_node;
-    prepared_lua_function_t m_process_way;
-    prepared_lua_function_t m_process_relation;
-    prepared_lua_function_t m_select_relation_members;
+    prepared_lua_function_t m_process_node{};
+    prepared_lua_function_t m_process_way{};
+    prepared_lua_function_t m_process_relation{};
+    prepared_lua_function_t m_select_relation_members{};
 
     calling_context m_calling_context = calling_context::main;
 

src/output-gazetteer.cpp

@@ -21,6 +21,8 @@
 #include <memory>
 #include <string>
 
+output_gazetteer_t::~output_gazetteer_t() = default;
+
 void output_gazetteer_t::delete_unused_classes(char osm_type, osmid_t osm_id)
 {
     if (get_options()->append) {

src/output-gazetteer.hpp

@@ -28,32 +28,39 @@ struct middle_query_t;
 
 class output_gazetteer_t : public output_t
 {
-    output_gazetteer_t(output_gazetteer_t const *other,
-                       std::shared_ptr<middle_query_t> const &cloned_mid,
-                       std::shared_ptr<db_copy_thread_t> const &copy_thread)
-    : output_t(cloned_mid, other->m_thread_pool, *other->get_options()),
-      m_copy(copy_thread), m_proj(other->get_options()->projection),
-      m_osmium_buffer(PLACE_BUFFER_SIZE, osmium::memory::Buffer::auto_grow::yes)
-    {}
-
 public:
+    /// Constructor for new objects
     output_gazetteer_t(std::shared_ptr<middle_query_t> const &mid,
                        std::shared_ptr<thread_pool_t> thread_pool,
                        options_t const &options,
                        std::shared_ptr<db_copy_thread_t> const &copy_thread)
     : output_t(mid, std::move(thread_pool), options), m_copy(copy_thread),
-      m_proj(options.projection),
-      m_osmium_buffer(PLACE_BUFFER_SIZE, osmium::memory::Buffer::auto_grow::yes)
+      m_proj(options.projection)
     {
         m_style.load_style(options.style);
     }
 
+    /// Constructor for cloned objects
+    output_gazetteer_t(output_gazetteer_t const *other,
+                       std::shared_ptr<middle_query_t> const &mid,
+                       std::shared_ptr<db_copy_thread_t> const &copy_thread)
+    : output_t(other, mid), m_copy(copy_thread),
+      m_proj(other->get_options()->projection)
+    {}
+
+    output_gazetteer_t(output_gazetteer_t const &) = delete;
+    output_gazetteer_t &operator=(output_gazetteer_t const &) = delete;
+
+    output_gazetteer_t(output_gazetteer_t &&) = delete;
+    output_gazetteer_t &operator=(output_gazetteer_t &&) = delete;
+
+    ~output_gazetteer_t() override;
+
     std::shared_ptr<output_t>
     clone(std::shared_ptr<middle_query_t> const &mid,
           std::shared_ptr<db_copy_thread_t> const &copy_thread) const override
     {
-        return std::shared_ptr<output_t>(
-            new output_gazetteer_t{this, mid, copy_thread});
+        return std::make_shared<output_gazetteer_t>(this, mid, copy_thread);
     }
 
     void start() override;
@@ -93,7 +100,8 @@ class output_gazetteer_t : public output_t
     gazetteer_style_t m_style;
 
     std::shared_ptr<reprojection> m_proj;
-    osmium::memory::Buffer m_osmium_buffer;
+    osmium::memory::Buffer m_osmium_buffer{
+        PLACE_BUFFER_SIZE, osmium::memory::Buffer::auto_grow::yes};
 };
 
 #endif // OSM2PGSQL_OUTPUT_GAZETTEER_HPP

src/output-null.cpp

@@ -9,11 +9,11 @@
 
 #include "output-null.hpp"
 
-std::shared_ptr<output_t>
-output_null_t::clone(std::shared_ptr<middle_query_t> const &mid,
-                     std::shared_ptr<db_copy_thread_t> const &) const
+std::shared_ptr<output_t> output_null_t::clone(
+    std::shared_ptr<middle_query_t> const & /*mid*/,
+    std::shared_ptr<db_copy_thread_t> const & /*copy_thread*/) const
 {
-    return std::make_shared<output_null_t>(mid, m_thread_pool, *get_options());
+    return std::make_shared<output_null_t>(*this);
 }
 
 output_null_t::output_null_t(std::shared_ptr<middle_query_t> const &mid,

src/output-null.hpp

@@ -22,6 +22,12 @@ class output_null_t : public output_t
                   std::shared_ptr<thread_pool_t> thread_pool,
                   options_t const &options);
 
+    output_null_t(output_null_t const &) = default;
+    output_null_t &operator=(output_null_t const &) = default;
+
+    output_null_t(output_null_t &&) = default;
+    output_null_t &operator=(output_null_t &&) = default;
+
     ~output_null_t() override;
 
     std::shared_ptr<output_t>

src/output-pgsql.cpp

@@ -416,8 +416,7 @@ std::shared_ptr<output_t> output_pgsql_t::clone(
     std::shared_ptr<middle_query_t> const &mid,
     std::shared_ptr<db_copy_thread_t> const &copy_thread) const
 {
-    return std::shared_ptr<output_t>(
-        new output_pgsql_t{this, mid, copy_thread});
+    return std::make_shared<output_pgsql_t>(this, mid, copy_thread);
 }
 
 output_pgsql_t::output_pgsql_t(
@@ -481,8 +480,7 @@ output_pgsql_t::output_pgsql_t(
 output_pgsql_t::output_pgsql_t(
     output_pgsql_t const *other, std::shared_ptr<middle_query_t> const &mid,
     std::shared_ptr<db_copy_thread_t> const &copy_thread)
-: output_t(mid, other->m_thread_pool, *other->get_options()),
-  m_tagtransform(other->m_tagtransform->clone()),
+: output_t(other, mid), m_tagtransform(other->m_tagtransform->clone()),
   m_enable_way_area(other->m_enable_way_area),
   m_proj(get_options()->projection),
   m_expire(get_options()->expire_tiles_zoom,

src/output-pgsql.hpp

@@ -27,10 +27,6 @@
 
 class output_pgsql_t : public output_t
 {
-    output_pgsql_t(output_pgsql_t const *other,
-                   std::shared_ptr<middle_query_t> const &mid,
-                   std::shared_ptr<db_copy_thread_t> const &copy_thread);
-
 public:
     enum table_id
     {
@@ -41,11 +37,23 @@ class output_pgsql_t : public output_t
         t_MAX
     };
 
+    /// Constructor for new objects
     output_pgsql_t(std::shared_ptr<middle_query_t> const &mid,
                    std::shared_ptr<thread_pool_t> thread_pool,
                    options_t const &options,
                    std::shared_ptr<db_copy_thread_t> const &copy_thread);
 
+    /// Constructor for cloned objects
+    output_pgsql_t(output_pgsql_t const *other,
+                   std::shared_ptr<middle_query_t> const &mid,
+                   std::shared_ptr<db_copy_thread_t> const &copy_thread);
+
+    output_pgsql_t(output_pgsql_t const &) = delete;
+    output_pgsql_t &operator=(output_pgsql_t const &) = delete;
+
+    output_pgsql_t(output_pgsql_t &&) = delete;
+    output_pgsql_t &operator=(output_pgsql_t &&) = delete;
+
     ~output_pgsql_t() override;
 
     std::shared_ptr<output_t>

src/output.cpp

@@ -66,7 +66,12 @@ output_t::output_t(std::shared_ptr<middle_query_t> mid,
                    options_t const &options)
 : m_mid(std::move(mid)), m_options(&options),
   m_thread_pool(std::move(thread_pool))
+{}
 
+output_t::output_t(output_t const *other, std::shared_ptr<middle_query_t> mid)
+: m_mid(std::move(mid)), m_options(other->m_options),
+  m_thread_pool(other->m_thread_pool),
+  m_output_requirements(other->m_output_requirements)
 {}
 
 output_t::~output_t() = default;

src/output.hpp

@@ -32,17 +32,24 @@ struct middle_query_t;
 class output_t
 {
 public:
+    /// Factory method for creating instances of classes derived from output_t.
     static std::shared_ptr<output_t>
     create_output(std::shared_ptr<middle_query_t> const &mid,
                   std::shared_ptr<thread_pool_t> thread_pool,
                   options_t const &options);
 
-    output_t(std::shared_ptr<middle_query_t> mid,
-             std::shared_ptr<thread_pool_t> thread_pool,
-             options_t const &options);
+    output_t(output_t const &) = default;
+    output_t &operator=(output_t const &) = default;
+
+    output_t(output_t &&) = default;
+    output_t &operator=(output_t &&) = default;
 
     virtual ~output_t();
 
+    /**
+     * This function clones instances of derived classes of output_t, it must
+     * be implemented in derived classes.
+     */
     virtual std::shared_ptr<output_t>
     clone(std::shared_ptr<middle_query_t> const &mid,
           std::shared_ptr<db_copy_thread_t> const &copy_thread) const = 0;
@@ -96,8 +103,23 @@ class output_t
 private:
     std::shared_ptr<middle_query_t> m_mid;
     options_t const *m_options;
+    std::shared_ptr<thread_pool_t> m_thread_pool;
 
 protected:
+    /**
+     * Constructor used for creating a new object using the create_output()
+     * function.
+     */
+    output_t(std::shared_ptr<middle_query_t> mid,
+             std::shared_ptr<thread_pool_t> thread_pool,
+             options_t const &options);
+
+    /**
+     * Constructor used for cloning an existing output using clone(). It gets
+     * a new middle query pointer, everything else is copied over.
+     */
+    output_t(output_t const *other, std::shared_ptr<middle_query_t> mid);
+
     thread_pool_t &thread_pool() const noexcept
     {
         assert(m_thread_pool);
@@ -112,7 +134,6 @@ class output_t
 
     const options_t *get_options() const noexcept { return m_options; };
 
-    std::shared_ptr<thread_pool_t> m_thread_pool;
     output_requirements m_output_requirements{};
 };