Merge pull request #304 from Woazboat/cleanup

Cleanup & code de-duplication

include/cgimap/api06/handler_utils.hpp

@@ -5,12 +5,12 @@
 #include "cgimap/request.hpp"
 #include "cgimap/api06/id_version.hpp"
 #include <vector>
-#include <string>
+#include <string_view>
 
 namespace api06 {
 
 std::vector<id_version> parse_id_list_params(request &req,
-                                             const std::string &param_name);
+                                             std::string_view param_name);
 }
 
 #endif /* API06_HANDLER_UTILS_HPP */

include/cgimap/backend/apidb/quad_tile.hpp

@@ -11,11 +11,10 @@ std::vector<tile_id_t> tiles_for_area(double minlat, double minlon, double maxla
 /* following functions liberally nicked from TomH's quad_tile
  * library.
  */
-inline unsigned int xy2tile(unsigned int x, unsigned int y) {
+constexpr unsigned int xy2tile(unsigned int x, unsigned int y) {
   unsigned int tile = 0;
-  int i;
 
-  for (i = 15; i >= 0; --i) {
+  for (int i = 15; i >= 0; --i) {
     tile = (tile << 1) | ((x >> i) & 1);
     tile = (tile << 1) | ((y >> i) & 1);
   }

include/cgimap/backend/apidb/readonly_pgsql_selection.hpp

@@ -23,7 +23,7 @@ class readonly_pgsql_selection : public data_selection {
 
 public:
   readonly_pgsql_selection(Transaction_Owner_Base& to);
-  ~readonly_pgsql_selection();
+  ~readonly_pgsql_selection() override = default;
 
   void write_nodes(output_formatter &formatter) override;
   void write_ways(output_formatter &formatter) override;
@@ -75,8 +75,8 @@ class readonly_pgsql_selection : public data_selection {
   class factory : public data_selection::factory {
   public:
     factory(const boost::program_options::variables_map &);
-    virtual ~factory();
-    std::unique_ptr<data_selection> make_selection(Transaction_Owner_Base&) override;
+    ~factory() override = default;
+    std::unique_ptr<data_selection> make_selection(Transaction_Owner_Base&) const override;
     std::unique_ptr<Transaction_Owner_Base> get_default_transaction() override;
 
   private:
@@ -86,18 +86,18 @@ class readonly_pgsql_selection : public data_selection {
   };
 
 private:
-  std::set< osm_changeset_id_t > extract_changeset_ids(pqxx::result& result);
+  std::set< osm_changeset_id_t > extract_changeset_ids(const pqxx::result& result) const;
   void fetch_changesets(const std::set< osm_changeset_id_t >& ids, std::map<osm_changeset_id_t, changeset> & cc);
 
   Transaction_Manager m;
 
   // true if we want to include changeset discussions along with
-  // the changesets themselves. defaults to false.
-  bool include_changeset_discussions;
+  // the changesets themselves.
+  bool include_changeset_discussions { false };
 
   // true if the user is a moderator and we should include redacted historical
   // versions in the responses.
-  bool m_redactions_visible;
+  bool m_redactions_visible { false };
 
   // the set of selected nodes, ways and relations
   std::set<osm_changeset_id_t> sel_changesets;

include/cgimap/backend/apidb/transaction_manager.hpp

@@ -14,10 +14,10 @@
 class Transaction_Owner_Base
 {
 public:
-  Transaction_Owner_Base() {}
+  Transaction_Owner_Base() = default;
   virtual pqxx::transaction_base& get_transaction() = 0;
   virtual std::set<std::string>& get_prep_stmt() = 0;
-  virtual ~Transaction_Owner_Base() {}
+  virtual ~Transaction_Owner_Base() = default;
 };
 
 
@@ -27,7 +27,7 @@ class Transaction_Owner_ReadOnly : public Transaction_Owner_Base
   explicit Transaction_Owner_ReadOnly(pqxx::connection &conn, std::set<std::string> &prep_stmt);
   pqxx::transaction_base& get_transaction() override;
   std::set<std::string>& get_prep_stmt() override;
-  ~Transaction_Owner_ReadOnly() {}
+  ~Transaction_Owner_ReadOnly() override = default;
 
 private:
   pqxx::read_transaction m_txn;
@@ -41,7 +41,7 @@ class Transaction_Owner_ReadWrite : public Transaction_Owner_Base
   explicit Transaction_Owner_ReadWrite(pqxx::connection &conn, std::set<std::string> &prep_stmt);
   pqxx::transaction_base& get_transaction() override;
   std::set<std::string>& get_prep_stmt() override;
-  ~Transaction_Owner_ReadWrite() {}
+  ~Transaction_Owner_ReadWrite() override = default;
 
 private:
   pqxx::work m_txn;
@@ -54,7 +54,7 @@ class Transaction_Owner_Void : public Transaction_Owner_Base
   explicit Transaction_Owner_Void();
   pqxx::transaction_base& get_transaction() override;
   std::set<std::string>& get_prep_stmt() override;
-  ~Transaction_Owner_Void() {}
+  ~Transaction_Owner_Void() override = default;
 };
 
 

include/cgimap/bbox.hpp

@@ -7,11 +7,11 @@
  * Container for a simple lat/lon bounding box.
  */
 struct bbox {
-  double minlat, minlon, maxlat, maxlon;
+  double minlat{0.0}, minlon{0.0}, maxlat{0.0}, maxlon{0.0};
 
   bbox(double minlat_, double minlon_, double maxlat_, double maxlon_);
 
-  bbox();
+  constexpr bbox() = default;
 
   bool operator==(const bbox &) const;
 

include/cgimap/data_selection.hpp

@@ -186,7 +186,7 @@ class data_selection {
 
     /// get a handle to a selection which can be used to build up
     /// a working set of data.
-    virtual std::unique_ptr<data_selection> make_selection(Transaction_Owner_Base&) = 0;
+    virtual std::unique_ptr<data_selection> make_selection(Transaction_Owner_Base&) const = 0;
 
     virtual std::unique_ptr<Transaction_Owner_Base> get_default_transaction() = 0;
   };

include/cgimap/fcgi_request.hpp

@@ -7,7 +7,7 @@
 
 struct fcgi_request : public request {
   fcgi_request(int socket, const std::chrono::system_clock::time_point &now);
-  virtual ~fcgi_request();
+  ~fcgi_request() override;
   const char *get_param(const char *key) const override;
   const std::string get_payload() override;
 
@@ -23,7 +23,7 @@ struct fcgi_request : public request {
   void dispose() override;
 
 protected:
-  void write_header_info(int status, const request::headers_t &headers) override;
+  void write_header_info(int status, const http::headers_t &headers) override;
   output_buffer& get_buffer_internal() override;
   void finish_internal() override;
 

include/cgimap/http.hpp

@@ -30,6 +30,16 @@ namespace http {
     OPTIONS = 0b10000
   };
 
+  using headers_t = std::vector<std::pair<std::string, std::string> >;
+
+  std::string format_header(int status, const headers_t &headers);
+
+  /**
+   * return a static string description for an HTTP status code.
+   */
+  const char *status_message(int code);
+
+
 /**
  * Base class for HTTP protocol related exceptions.
  *
@@ -41,22 +51,18 @@ class exception : public std::exception {
   /// http://en.wikipedia.org/wiki/List_of_HTTP_status_codes
   const int code_;
 
-  /// the header is a short description of the code, mainly for
-  /// human consumption.
-  const std::string header_;
-
   /// specific error message, meant entirely for humans to read.
   const std::string message_;
 
 protected:
-  exception(int c, const std::string &h, const std::string &m);
+  exception(int c, std::string m);
 
 public:
-  virtual ~exception() noexcept;
+  ~exception() noexcept override = default;
 
   int code() const;
-  const std::string &header() const;
-  const char *what() const noexcept;
+  const char* header() const;
+  const char* what() const noexcept override;
 };
 
 /**
@@ -236,7 +242,7 @@ class encoding {
   const std::string name_;
 
 public:
-  encoding(const std::string &name) : name_(name){}
+  encoding(std::string name) : name_(std::move(name)){}
   virtual ~encoding() = default;
 
   const std::string &name() const { return name_; };
@@ -282,10 +288,12 @@ std::unique_ptr<ZLibBaseDecompressor> get_content_encoding_handler(const std::st
 
 // allow bitset-like operators on methods
 inline method operator|(method a, method b) {
-  return static_cast<method>(static_cast<uint8_t>(a) | static_cast<uint8_t>(b));
+  return static_cast<method>(static_cast<std::underlying_type_t<method>>(a) |
+                             static_cast<std::underlying_type_t<method>>(b));
 }
 inline method operator&(method a, method b) {
-  return static_cast<method>(static_cast<uint8_t>(a) & static_cast<uint8_t>(b));
+  return static_cast<method>(static_cast<std::underlying_type_t<method>>(a) &
+                             static_cast<std::underlying_type_t<method>>(b));
 }
 inline method& operator|=(method& a, method b)
 {

include/cgimap/output_buffer.hpp

@@ -1,11 +1,14 @@
 #ifndef OUTPUT_BUFFER_HPP
 #define OUTPUT_BUFFER_HPP
 
+#include <string_view>
+
 /**
  * Implement this interface to provide custom output.
  */
 struct output_buffer {
   virtual int write(const char *buffer, int len) = 0;
+  virtual int write(std::string_view str) { return write(str.data(), str.size()); }
   virtual int written() = 0;
   virtual int close() = 0;
   virtual void flush() = 0;

include/cgimap/rate_limiter.hpp

@@ -18,7 +18,7 @@ struct rate_limiter {
 
 struct null_rate_limiter
   : public rate_limiter {
-  ~null_rate_limiter();
+  ~null_rate_limiter() override;
   std::tuple<bool, int> check(const std::string &key, bool moderator) override;
   void update(const std::string &key, int bytes, bool moderator) override;
 };
@@ -30,7 +30,7 @@ class memcached_rate_limiter
    * Methods.
    */
   memcached_rate_limiter(const boost::program_options::variables_map &options);
-  ~memcached_rate_limiter();
+  ~memcached_rate_limiter() override;
   std::tuple<bool, int> check(const std::string &key, bool moderator) override;
   void update(const std::string &key, int bytes, bool moderator) override;
 

include/cgimap/request.hpp

@@ -26,8 +26,8 @@ struct output_buffer;
  * body.
  */
 struct request {
-  request();
-  virtual ~request();
+  request() = default;
+  virtual ~request() = default;
 
   // get the value associated with a key in the request headers. returns NULL if
   // the key could not be found. this function can be called at any time.
@@ -89,12 +89,11 @@ struct request {
   http::method methods() const;
 
 protected:
-  using headers_t = std::vector<std::pair<std::string, std::string> >;
 
   // this is called once, the first time an output function is called. the
   // implementing output system may use this to write out the complete set of
   // status & header information.
-  virtual void write_header_info(int status, const headers_t &headers) = 0;
+  virtual void write_header_info(int status, const http::headers_t &headers) = 0;
 
   // internal functions.
   // TODO: this is really bad design and indicates this should probably use
@@ -113,22 +112,22 @@ struct request {
     status_BODY = 2,
     status_FINISHED = 3
   };
-  workflow_status m_workflow_status;
+  workflow_status m_workflow_status{status_NONE};
 
   // function to check and update the workflow
   void check_workflow(workflow_status this_stage);
 
   // the HTTP status code
-  int m_status;
+  int m_status{500};
 
   // the headers to be written in the response
-  headers_t m_headers;
+  http::headers_t m_headers;
 
   // the headers to be written in the response if process was successful
-  headers_t m_success_headers;
+  http::headers_t m_success_headers;
 
   // allowed methods, to be returned to the client in the CORS headers.
-  http::method m_methods;
+  http::method m_methods{http::method::GET | http::method::HEAD | http::method::OPTIONS};
 };
 
 #endif /* REQUEST_HPP */

include/cgimap/request_helpers.hpp

@@ -32,11 +32,6 @@ std::string get_request_path(request &req);
  */
 std::unique_ptr<http::encoding> get_encoding(request &req);
 
-/**
- * return a static string description for an HTTP status code.
- */
-const char *status_message(int code);
-
 /**
  * return shared pointer to a buffer object which can be
  * used to write to the response body.

include/cgimap/router.hpp

@@ -112,7 +112,7 @@ struct match_string : public ops<match_string> {
   match_string(const char *s);
 
   // copy just copies the held string
-  inline match_string(const match_string &m) : str(m.str) {}
+  inline match_string(const match_string &m) = default;
 
   match_type match(part_iterator &begin, const part_iterator &end) const;
 

include/cgimap/routes.hpp

@@ -23,8 +23,8 @@ class routes {
   routes& operator=(routes &&) = default;
 
   /**
-         * returns the handler which matches a request, or throws a 404 error.
-         */
+   * returns the handler which matches a request, or throws a 404 error.
+   */
   handler_ptr_t operator()(request &req) const;
 
 private:

include/cgimap/types.hpp

@@ -26,12 +26,17 @@ enum class osm_user_role_t {
 
 // OSMChange message operations
 enum class operation {
-        op_undefined = 0, op_create = 1, op_modify = 2, op_delete = 3
+  op_undefined = 0,
+  op_create = 1,
+  op_modify = 2,
+  op_delete = 3
 };
 
 // OSMChange message object type
 enum class object_type {
-  node = 1, way = 2, relation = 3
+  node = 1,
+  way = 2,
+  relation = 3
 };
 
 

src/api06/handler_utils.cpp

@@ -23,16 +23,6 @@ using std::pair;
 namespace qi = boost::spirit::qi;
 namespace standard = boost::spirit::standard;
 
-namespace {
-struct first_equals {
-  first_equals(const std::string &k) : m_key(k) {}
-  bool operator()(const pair<string, string> &v) const {
-    return v.first == m_key;
-  }
-  string m_key;
-};
-} // anonymous namespace
-
 BOOST_FUSION_ADAPT_STRUCT(
   api06::id_version,
   (uint64_t, id)
@@ -80,11 +70,11 @@ bool valid_string(const std::string& str)
 		     [](char c){ return c >= 0 && c <= UCHAR_MAX; });
 }
 
-vector<id_version> parse_id_list_params(request &req, const string &param_name) {
+vector<id_version> parse_id_list_params(request &req, std::string_view param_name) {
 
   string decoded = http::urldecode(get_query_string(req));
   const vector<pair<string, string> > params = http::parse_params(decoded);
-  auto itr = std::find_if(params.begin(), params.end(), first_equals(param_name));
+  auto itr = std::find_if(params.begin(), params.end(), [&param_name](auto& x){ return x.first == param_name; });
 
   if (itr == params.end())
     return {};

src/backend/apidb/common_pgsql_selection.cpp

@@ -224,7 +224,8 @@ void extract(
     extract_elem(row, elem, cc);
     extra.extract(row);
     extract_tags(row, tags);
-    notify(elem);     // let callback function know about a new element we're processing
+    if (notify)
+      notify(elem);     // let callback function know about a new element we're processing
     T::write(formatter, elem, extra, tags);
   }
 }