<p><b>@joto</b> commented on this pull request.</p>
<p>I didn't get through everything, but here is what I got so far.</p><hr>
<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/966#discussion_r345836321">tests/common-cleanup.hpp</a>:</p>
<pre style='color:#555'>> namespace cleanup {
-// RAII structure to remove a file upon destruction. doesn't create or do
-// anything to the file when it constructs.
-struct file {
- file(const std::string &filename);
- ~file();
+/**
+ * RAII structure to remove a file upon destruction.
+ *
+ * Per defualt will also make sure that the file does not exist
</pre>
<p>s/defualt/default/</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/966#discussion_r345836455">tests/common-cleanup.hpp</a>:</p>
<pre style='color:#555'>> + * RAII structure to remove a file upon destruction.
+ *
+ * Per defualt will also make sure that the file does not exist
+ * when it is constructed.
+ */
+class file_t
+{
+public:
+ file_t(std::string const &filename, bool remove_on_construct = true)
+ : m_filename(filename)
+ {
+ if (remove_on_construct)
+ delete_file(false);
+ }
+
+ ~file_t() { delete_file(true); }
</pre>
<p>Destructors should be noexcept</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/966#discussion_r345836726">tests/common-cleanup.hpp</a>:</p>
<pre style='color:#555'>> private:
- // name of the file to be deleted upon destruction
- std::string m_filename;
-};
+ void delete_file(bool warn)
</pre>
<p>Should be noexcept, because it is called from destructor.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/966#discussion_r345838943">tests/common-import.hpp</a>:</p>
<pre style='color:#555'>> +namespace db {
+
+/**
+ * Convenience class around tempdb_t that offers functions for
+ * data import from file and strings.
+ */
+class import_t
+{
+public:
+ void run_import(options_t options, char const *data,
+ std::string const &fmt = "opl")
+ {
+ options.database_options = m_db.db_options();
+
+ // setup the middle
+ std::shared_ptr<middle_t> middle(new middle_ram_t(&options));
</pre>
<p>make_shared?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/966#discussion_r345839391">tests/common-import.hpp</a>:</p>
<pre style='color:#555'>> +
+ osmdata.start();
+
+ test_parse_t parser(options.bbox, options.append, &osmdata);
+
+ parser.stream_buffer(data, fmt);
+
+ osmdata.stop();
+ }
+
+ void run_file(options_t options, char const *file = nullptr)
+ {
+ options.database_options = m_db.db_options();
+
+ // setup the middle
+ std::shared_ptr<middle_t> middle(new middle_ram_t(&options));
</pre>
<p>make_shared? More of the same below</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/966#discussion_r345843032">tests/common-pg.hpp</a>:</p>
<pre style='color:#555'>>
-struct conn
- : public boost::noncopyable,
- public std::enable_shared_from_this<conn> {
+ ~result_t() { PQclear(m_result); }
</pre>
<p>Destructor should be noexcept. In fact all functions in this class could probably be noexcept.</p>
<p>Same issue below.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/966#discussion_r345846155">tests/common-pg.hpp</a>:</p>
<pre style='color:#555'>> - * Assert that the database has a certain table_name
- * \param[in] table_name Name of the table to check, optionally schema-qualified
- * \throws std::runtime_error if missing the table
- */
- void assert_has_table(const std::string &table_name);
+class tempdb_t
+{
+public:
+ tempdb_t()
+ {
+ conn_t conn("dbname=postgres");
+
+ m_db_name =
+ (boost::format("osm2pgsql-test-%1%-%2%") % getpid() % time(nullptr))
+ .str();
+ conn.exec(boost::format("DROP DATABASE IF EXISTS \"%1%\"") % m_db_name);
</pre>
<p>Use C++11 raw string literals? <code>R"SQL(DROP DATABASE IF EXISTS "%1%")SQL"</code></p>
<p>Is a bit unfamiliar, but saves on escaping...</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/966#discussion_r345849760">tests/test-db-copy-mgr.cpp</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,192 @@
+#include <string>
+#include <vector>
+
+#include "catch.hpp"
+
+#include "common-pg.hpp"
+#include "db-copy.hpp"
+
+using fmt = boost::format;
</pre>
<p>Used only twice in this file plus twice more in <code>test-output-gazetteer.cpp</code>. Either remove or maybe put into common include so <code>common-pg.hpp</code> can also use it, which uses <code>boost::format</code> a lot.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/966#discussion_r345851425">tests/test-db-copy-mgr.cpp</a>:</p>
<pre style='color:#555'>> + conn.exec(fmt("CREATE TABLE test_copy_mgr (id int8%1%%2%)") %
+ (cols.empty() ? "" : ",") % cols);
+
+ auto table = std::make_shared<db_target_descr_t>();
+ table->name = "test_copy_mgr";
+ table->id = "id";
+
+ return table;
+}
+
+template <typename... ARGS>
+void add_row(db_copy_mgr_t &mgr, std::shared_ptr<db_target_descr_t> t,
+ ARGS &&... args)
+{
+ mgr.new_line(t);
+ mgr.add_columns(args...);
</pre>
<p><code>std::forward<ARGS>(args)...</code> ?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/966#discussion_r345901863">tests/test-db-copy-mgr.cpp</a>:</p>
<pre style='color:#555'>> + std::vector<std::pair<std::string, std::string>> values)
+{
+ mgr.new_line(t);
+
+ mgr.add_column(id);
+ mgr.new_hash();
+ for (auto const &v : values) {
+ mgr.add_hash_elem(v.first, v.second);
+ }
+ mgr.finish_hash();
+ mgr.finish_line();
+
+ mgr.sync();
+}
+
+static void check_row(std::vector<std::string> row)
</pre>
<p><code>std::vector<...> const & row</code></p>
<p>Alternatively use<code> std::initializer_list</code> because these strings in the argument are all literals.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/966#discussion_r345902148">tests/test-db-copy-mgr.cpp</a>:</p>
<pre style='color:#555'>> +{
+ mgr.new_line(t);
+ mgr.add_column(id);
+ mgr.new_array();
+ for (auto v : values) {
+ mgr.add_array_elem(v);
+ }
+ mgr.finish_array();
+ mgr.finish_line();
+
+ mgr.sync();
+}
+
+static void add_hash(db_copy_mgr_t &mgr, std::shared_ptr<db_target_descr_t> t,
+ int id,
+ std::vector<std::pair<std::string, std::string>> values)
</pre>
<p><code>std::vector<...> const &values</code></p>
<hr>
<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/966#discussion_r345902615">tests/test-db-copy-mgr.cpp</a>:</p>
<pre style='color:#555'>> +}
+
+template <typename... ARGS>
+void add_row(db_copy_mgr_t &mgr, std::shared_ptr<db_target_descr_t> t,
+ ARGS &&... args)
+{
+ mgr.new_line(t);
+ mgr.add_columns(args...);
+ mgr.finish_line();
+
+ mgr.sync();
+}
+
+template <typename T>
+void add_array(db_copy_mgr_t &mgr, std::shared_ptr<db_target_descr_t> t, int id,
+ std::vector<T> values)
</pre>
<p><code>const std::vector<T> const &values</code></p>
<hr>
<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/966#discussion_r345907609">tests/common-pg.hpp</a>:</p>
<pre style='color:#555'>> + result_t res = query(cmd);
+ REQUIRE(res.status() == PGRES_TUPLES_OK);
+ REQUIRE(res.num_tuples() == 1);
+
+ std::string str = res.get_value(0, 0);
+ return boost::lexical_cast<T>(str);
+ }
+
+ void assert_double(double expected, std::string const &cmd) const
+ {
+ REQUIRE(Approx(expected) == require_scalar<double>(cmd));
+ }
+
+ void assert_null(std::string const &cmd) const
+ {
+ REQUIRE(require_scalar<std::string>(cmd) == "");
</pre>
<p>Does this check for <code>NULL</code> or empty string?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/966#discussion_r345909467">tests/test-middle.cpp</a>:</p>
<pre style='color:#555'>> +#include <osmium/osm/crc_zlib.hpp>
+
+#include "middle-pgsql.hpp"
+#include "middle-ram.hpp"
+
+#include "common-options.hpp"
+#include "common-pg.hpp"
+
+static pg::tempdb_t db;
+
+namespace {
+
+/// Simple osmium buffer to store object with some convenience.
+struct test_buffer_t
+{
+ size_t add_node(osmid_t id, double lat, double lon)
</pre>
<p>Here order is <code>lat, lon</code>, below <code>_location(lon, lat)</code>. Could be confusing. Maybe use <code>osmium::Location</code> as parameter?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/966#discussion_r345910803">tests/test-middle.cpp</a>:</p>
<pre style='color:#555'>> + template <typename T>
+ T const &get(size_t pos) const
+ {
+ return buf.get<T>(pos);
+ }
+
+ template <typename T>
+ T &get(size_t pos)
+ {
+ return buf.get<T>(pos);
+ }
+
+ osmium::memory::Buffer buf{4096, osmium::memory::Buffer::auto_grow::yes};
+};
+
+void expect_location(osmium::Location loc, osmium::Node const &expected)
</pre>
<p>Do we need this? Because <code>Location</code> internally uses integers just comparing locations could be enough.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/966#discussion_r345912304">tests/test-middle.cpp</a>:</p>
<pre style='color:#555'>> + buffer.get<osmium::Way>(buffer.add_way(3, {1234})).nodes();
+
+ // get it back
+ REQUIRE(mid_q->nodes_get_list(&(nodes)) == nodes.size());
+ expect_location(nodes[0].location(), node);
+
+ // other nodes are not retrievable
+ auto &n2 =
+ buffer.get<osmium::Way>(buffer.add_way(3, {1, 2, 1235})).nodes();
+ REQUIRE(mid_q->nodes_get_list(&(n2)) == 0);
+ }
+
+ SECTION("Set and retrieve a single way")
+ {
+ osmid_t way_id = 1;
+ double lat = 12.3456789;
</pre>
<p>These can be <code>const</code>. Probably lots of other places in the code where <code>const</code> could be added.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/966#discussion_r345912746">tests/test-middle.cpp</a>:</p>
<pre style='color:#555'>> + mid->flush(osmium::item_type::relation);
+
+ // get it back
+ osmium::memory::Buffer outbuf{4096,
+ osmium::memory::Buffer::auto_grow::yes};
+
+ REQUIRE(mid_q->ways_get(way_id, outbuf));
+
+ auto &way = outbuf.get<osmium::Way>(0);
+
+ CHECK(way.id() == way_id);
+ REQUIRE(way.nodes().size() == nds.size());
+
+ REQUIRE(mid_q->nodes_get_list(&(way.nodes())) == nds.size());
+ for (osmid_t i = 1; i <= 10; ++i) {
+ CHECK(way.nodes()[(size_t)i - 1].ref() == i);
</pre>
<p><code>static_cast<std::size_t>(i)</code></p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/openstreetmap/osm2pgsql/pull/966?email_source=notifications&email_token=AA6353QIYBEKQZBUXHTGU63QTQ7CPA5CNFSM4JL4CPO2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCLNQY4Q#pullrequestreview-316345458">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AA6353QSC6EMYFW7D7MMN53QTQ7CPANCNFSM4JL4CPOQ">unsubscribe</a>.<img src="https://github.com/notifications/beacon/AA6353S2FIKF6FUUEJJNOQ3QTQ7CPA5CNFSM4JL4CPO2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCLNQY4Q.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/osm2pgsql/pull/966?email_source=notifications\u0026email_token=AA6353QIYBEKQZBUXHTGU63QTQ7CPA5CNFSM4JL4CPO2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCLNQY4Q#pullrequestreview-316345458",
"url": "https://github.com/openstreetmap/osm2pgsql/pull/966?email_source=notifications\u0026email_token=AA6353QIYBEKQZBUXHTGU63QTQ7CPA5CNFSM4JL4CPO2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCLNQY4Q#pullrequestreview-316345458",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>