Merge pull request #1190 from joto/cleanup-options-handling

Cleanup options handling

Author: lonvia

src/geometry-processor.hpp

@@ -13,7 +13,7 @@
 #include "tagtransform.hpp"
 
 struct middle_query_t;
-struct options_t;
+class options_t;
 class reprojection;
 
 struct geometry_processor

src/middle-ram.hpp

@@ -16,7 +16,7 @@
 #include "middle.hpp"
 
 class node_ram_cache;
-struct options_t;
+class options_t;
 
 template <typename T, size_t N>
 class cache_block_t

src/node-persistent-cache.hpp

@@ -9,7 +9,7 @@
 #include "node-ram-cache.hpp"
 #include "osmtypes.hpp"
 
-struct options_t;
+class options_t;
 
 class node_persistent_cache
 {

src/options.cpp

@@ -8,7 +8,6 @@
 #include <cstring>
 #include <getopt.h>
 #include <osmium/version.hpp>
-#include <sstream>
 #include <stdexcept>
 #include <thread> // for number of threads
 
@@ -86,7 +85,7 @@ void short_usage(char *arg0)
                              "\t{} -h|--help\n"_format(program_name(arg0))};
 }
 
-void long_usage(char const *arg0, bool verbose = false)
+void long_usage(char const *arg0, bool verbose)
 {
     char const *const name = program_name(arg0);
 
@@ -257,33 +256,27 @@ void long_usage(char const *arg0, bool verbose = false)
 
 } // anonymous namespace
 
-database_options_t::database_options_t()
-: db(boost::none), username(boost::none), host(boost::none),
-  password(boost::none), port(boost::none)
-{}
-
 std::string database_options_t::conninfo() const
 {
-    std::ostringstream out;
+    std::string out{"fallback_application_name='osm2pgsql'"};
 
-    out << "fallback_application_name='osm2pgsql'";
-    if (db) {
-        out << " dbname='" << *db << "'";
+    if (!db.empty()) {
+        out += " dbname='{}'"_format(db);
     }
-    if (username) {
-        out << " user='" << *username << "'";
+    if (!username.empty()) {
+        out += " user='{}'"_format(username);
     }
-    if (password) {
-        out << " password='" << *password << "'";
+    if (!password.empty()) {
+        out += " password='{}'"_format(password);
     }
-    if (host) {
-        out << " host='" << *host << "'";
+    if (!host.empty()) {
+        out += " host='{}'"_format(host);
     }
-    if (port) {
-        out << " port='" << *port << "'";
+    if (!port.empty()) {
+        out += " port='{}'"_format(port);
     }
 
-    return out.str();
+    return out;
 }
 
 options_t::options_t()
@@ -302,8 +295,6 @@ options_t::options_t()
     }
 }
 
-options_t::~options_t() {}
-
 static osmium::Box parse_bbox(char const *bbox)
 {
     double minx, maxx, miny, maxy;
@@ -331,6 +322,8 @@ static osmium::Box parse_bbox(char const *bbox)
 
 options_t::options_t(int argc, char *argv[]) : options_t()
 {
+    bool help_verbose = false; // Will be set when -v/--verbose is set
+
     int c;
 
     //keep going while there are args left to handle
@@ -353,7 +346,7 @@ options_t::options_t(int argc, char *argv[]) : options_t()
             create = true;
             break;
         case 'v':
-            verbose = true;
+            help_verbose = true;
             break;
         case 's':
             slim = true;
@@ -472,21 +465,21 @@ options_t::options_t(int argc, char *argv[]) : options_t()
             extra_attributes = true;
             break;
         case 'k':
-            if (hstore_mode != HSTORE_NONE) {
+            if (hstore_mode != hstore_column::none) {
                 throw std::runtime_error{"You can not specify both --hstore "
                                          "(-k) and --hstore-all (-j)\n"};
             }
-            hstore_mode = HSTORE_NORM;
+            hstore_mode = hstore_column::norm;
             break;
         case 208:
             hstore_match_only = true;
             break;
         case 'j':
-            if (hstore_mode != HSTORE_NONE) {
+            if (hstore_mode != hstore_column::none) {
                 throw std::runtime_error{"You can not specify both --hstore "
                                          "(-k) and --hstore-all (-j)\n"};
             }
-            hstore_mode = HSTORE_ALL;
+            hstore_mode = hstore_column::all;
             break;
         case 'z':
             hstore_columns.emplace_back(optarg);
@@ -559,7 +552,7 @@ options_t::options_t(int argc, char *argv[]) : options_t()
 
     //they were looking for usage info
     if (long_usage_bool) {
-        long_usage(argv[0], verbose);
+        long_usage(argv[0], help_verbose);
         return;
     }
 
@@ -581,11 +574,9 @@ options_t::options_t(int argc, char *argv[]) : options_t()
     check_options();
 
     if (pass_prompt) {
-        char *prompt = simple_prompt("Password:", 100, 0);
-        if (prompt == nullptr) {
-            database_options.password = boost::none;
-        } else {
-            database_options.password = std::string(prompt);
+        char const *prompt = simple_prompt("Password:", 100, 0);
+        if (prompt != nullptr) {
+            database_options.password = prompt;
         }
     }
 }
@@ -605,15 +596,15 @@ void options_t::check_options()
         throw std::runtime_error{"--drop only makes sense with --slim.\n"};
     }
 
-    if (hstore_mode == HSTORE_NONE && hstore_columns.empty() &&
+    if (hstore_mode == hstore_column::none && hstore_columns.empty() &&
         hstore_match_only) {
         fprintf(stderr,
                 "Warning: --hstore-match-only only makes sense with --hstore, "
                 "--hstore-all, or --hstore-column; ignored.\n");
         hstore_match_only = false;
     }
 
-    if (enable_hstore_index && hstore_mode == HSTORE_NONE &&
+    if (enable_hstore_index && hstore_mode == hstore_column::none &&
         hstore_columns.empty()) {
         fprintf(stderr, "Warning: --hstore-add-index only makes sense with "
                         "hstore enabled.\n");

src/options.hpp

@@ -11,43 +11,46 @@
 #include <string>
 #include <vector>
 
-/* Variants for generation of hstore column */
-/* No hstore column */
-#define HSTORE_NONE 0
-/* create a hstore column for all tags which do not have an exclusive column */
-#define HSTORE_NORM 1
-/* create a hstore column for all tags */
-#define HSTORE_ALL 2
+/// Variants for generation of hstore column
+enum class hstore_column : char
+{
+    /// no hstore column
+    none = 0,
+    /// create hstore column for all tags that don't have their own column
+    norm = 1,
+    /// create hstore column for all tags
+    all = 2
+};
 
 /**
  * Database options, not specific to a table
  */
-class database_options_t
+struct database_options_t
 {
-public:
-    database_options_t();
-    boost::optional<std::string> db;
-    boost::optional<std::string> username;
-    boost::optional<std::string> host;
-    boost::optional<std::string> password;
-    boost::optional<std::string> port;
+    std::string db;
+    std::string username;
+    std::string host;
+    std::string password;
+    std::string port;
 
     std::string conninfo() const;
 };
 
 /**
  * Structure for storing command-line and other options
  */
-struct options_t
+class options_t
 {
 public:
-    // fixme: bring back old comment
+    /**
+     * Constructor setting default values for all options. Used for testing.
+     */
     options_t();
+
     /**
-     * Parse the options from the command line
+     * Constructor parsing the options from the command line.
      */
     options_t(int argc, char *argv[]);
-    virtual ~options_t();
 
     std::string prefix{"planet_osm"};         ///< prefix for table names
     std::shared_ptr<reprojection> projection; ///< SRS of projection
@@ -80,7 +83,7 @@ struct options_t
     std::string expire_tiles_filename{"dirty_tiles"};
 
     /// add an additional hstore column with objects key/value pairs, and what type of hstore column
-    int hstore_mode = HSTORE_NONE;
+    hstore_column hstore_mode = hstore_column::none;
 
     bool enable_hstore_index = false; ///< add an index on the hstore column
 
@@ -124,7 +127,6 @@ struct options_t
     std::string input_reader{"auto"};
     osmium::Box bbox;
     bool extra_attributes = false;
-    bool verbose = false;
 
     std::vector<std::string> input_files;
 

src/output-multi.hpp

@@ -17,11 +17,11 @@
 #include <memory>
 #include <string>
 
+class options_t;
 class table_t;
 class tagtransform_t;
 struct export_list;
 struct middle_query_t;
-struct options_t;
 
 class output_multi_t : public output_t
 {

src/output.cpp

@@ -73,7 +73,9 @@ parse_multi_single(pt::ptree const &conf,
     new_opts.tblsmain_index = conf.get("tablespace-index", "");
     new_opts.tblsmain_data = conf.get("tablespace-data", "");
 
-    override_if<int>(new_opts.hstore_mode, "enable-hstore", conf);
+    if (conf.get<bool>("enable-hstore", false)) {
+        new_opts.hstore_mode = hstore_column::norm;
+    }
     override_if<bool>(new_opts.enable_hstore_index, "enable-hstore-index",
                       conf);
     override_if<bool>(new_opts.enable_multi, "enable-multi", conf);

src/table.cpp

@@ -15,15 +15,15 @@
 
 table_t::table_t(std::string const &name, std::string const &type,
                  columns_t const &columns, hstores_t const &hstore_columns,
-                 int const srid, bool const append, int const hstore_mode,
+                 int const srid, bool const append, hstore_column hstore_mode,
                  std::shared_ptr<db_copy_thread_t> const &copy_thread)
 : m_target(std::make_shared<db_target_descr_t>(name.c_str(), "osm_id")),
   m_type(type), m_srid(fmt::to_string(srid)), m_append(append),
   m_hstore_mode(hstore_mode), m_columns(columns),
   m_hstore_columns(hstore_columns), m_copy(copy_thread)
 {
     // if we dont have any columns
-    if (m_columns.empty() && m_hstore_mode != HSTORE_ALL) {
+    if (m_columns.empty() && m_hstore_mode != hstore_column::all) {
         throw std::runtime_error{
             "No columns provided for table {}"_format(name)};
     }
@@ -99,7 +99,7 @@ void table_t::start(std::string const &conninfo, std::string const &table_space)
         }
 
         //add tags column
-        if (m_hstore_mode != HSTORE_NONE) {
+        if (m_hstore_mode != hstore_column::none) {
             sql += "\"tags\" hstore,";
         }
 
@@ -163,7 +163,7 @@ void table_t::generate_copy_column_list()
     }
 
     //add tags column and geom column
-    if (m_hstore_mode != HSTORE_NONE) {
+    if (m_hstore_mode != hstore_column::none) {
         m_target->rows += "tags,way";
         //or just the geom column
     } else {
@@ -264,7 +264,7 @@ void table_t::stop(bool updateable, bool enable_hstore_index,
         if (enable_hstore_index) {
             fmt::print(stderr, "Creating hstore indexes on {}\n",
                        m_target->name);
-            if (m_hstore_mode != HSTORE_NONE) {
+            if (m_hstore_mode != hstore_column::none) {
                 m_sql_conn->exec(
                     "CREATE INDEX ON {} USING GIN (tags) {}"_format(
                         m_target->name, tablespace_clause(table_space_index)));
@@ -303,18 +303,18 @@ void table_t::write_row(osmid_t id, taglist_t const &tags,
     // used to remember which columns have been written out already.
     std::vector<bool> used;
 
-    if (m_hstore_mode != HSTORE_NONE) {
+    if (m_hstore_mode != hstore_column::none) {
         used.assign(tags.size(), false);
     }
 
     //get the regular columns' values
-    write_columns(tags, m_hstore_mode == HSTORE_NORM ? &used : nullptr);
+    write_columns(tags, m_hstore_mode == hstore_column::norm ? &used : nullptr);
 
     //get the hstore columns' values
     write_hstore_columns(tags);
 
     //get the key value pairs for the tags column
-    if (m_hstore_mode != HSTORE_NONE) {
+    if (m_hstore_mode != hstore_column::none) {
         write_tags_column(tags, used);
     }
 

src/table.hpp

@@ -19,7 +19,7 @@ class table_t
 public:
     table_t(std::string const &name, std::string const &type,
             columns_t const &columns, hstores_t const &hstore_columns,
-            int const srid, bool const append, int const hstore_mode,
+            int const srid, bool const append, hstore_column hstore_mode,
             std::shared_ptr<db_copy_thread_t> const &copy_thread);
     table_t(table_t const &other,
             std::shared_ptr<db_copy_thread_t> const &copy_thread);
@@ -81,7 +81,7 @@ class table_t
     std::unique_ptr<pg_conn_t> m_sql_conn;
     std::string m_srid;
     bool m_append;
-    int m_hstore_mode;
+    hstore_column m_hstore_mode;
     columns_t m_columns;
     hstores_t m_hstore_columns;
     std::string m_table_space;

src/tagtransform-c.cpp

@@ -114,7 +114,7 @@ bool c_tagtransform_t::check_key(std::vector<taginfo> const &infos,
     // if we didn't find any tags that we wanted to export
     // and we aren't strictly adhering to the list
     if (!strict) {
-        if (m_options->hstore_mode != HSTORE_NONE) {
+        if (m_options->hstore_mode != hstore_column::none) {
             /* ... but if hstore_match_only is set then don't take this
                  as a reason for keeping the object */
             if (!m_options->hstore_match_only) {