<p></p>
<p><b>@lonvia</b> commented on this pull request.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/1176#discussion_r428283697">src/osmdata.cpp</a>:</p>
<pre style='color:#555'>> @@ -272,6 +272,12 @@ struct pending_threaded_processor : public pending_processor
                 throw;
             }
         }
+
+        for (auto const &clone : m_clones) {
+            for (auto const &clone_output : clone) {
+                clone_output->commit();
+            }
+        }
</pre>
<p>Shouldn't that even be moved to the workers?</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/1176#discussion_r428285884">src/osmdata.cpp</a>:</p>
<pre style='color:#555'>>          for (auto const &clone : m_clones) {
-            for (output_vec_t::const_iterator
-                     original_output = m_outputs.begin(),
-                     clone_output = clone.begin();
-                 original_output != m_outputs.end() &&
-                 clone_output != clone.end();
-                 ++original_output, ++clone_output) {
-                //merge the expire tree from this threads copy of output back
-                original_output->get()->merge_expire_trees(clone_output->get());
+            auto it = clone.begin();
+            for (auto const &output : m_outputs) {
+                assert(it != clone.end());
+                if (*it) {
+                    output->merge_expire_trees(it->get());
+                }
+                ++it;
</pre>
<p>How about switching the for loops? Might make the code even more understandable.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/1176#discussion_r428287159">src/osmdata.cpp</a>:</p>
<pre style='color:#555'>>  
     static osmid_t pop_id(pending_queue_t &queue, std::mutex &mutex)
     {
         osmid_t id = 0;
 
         std::lock_guard<std::mutex> const lock{mutex};
         if (!queue.empty()) {
-            id = queue.top();
-            queue.pop();
+            id = queue.back();
+            queue.pop_back();
</pre>
<p>This reverts the order, right? Could that be an issue?</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/1176#discussion_r428287577">src/dependency-manager.cpp</a>:</p>
<pre style='color:#555'>> @@ -43,18 +43,14 @@ bool full_dependency_manager_t::has_pending() const noexcept
     return !m_ways_pending_tracker.empty() || !m_rels_pending_tracker.empty();
 }
 
-void full_dependency_manager_t::process_pending(pending_processor &proc)
-{
-    osmid_t id;
-    while (id_tracker::is_valid(id = m_ways_pending_tracker.pop_mark())) {
-        proc.enqueue_way(id);
-    }
+idlist_t full_dependency_manager_t::get_ids(id_tracker& tracker) {
</pre>
<p>This commit is missing a clang-format run.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/1176#discussion_r428292346">src/dependency-manager.hpp</a>:</p>
<pre style='color:#555'>>      }
 
 private:
+    idlist_t get_ids(id_tracker& tracker);
</pre>
<p>can be static?</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/1176#discussion_r428301997">src/osmdata.cpp</a>:</p>
<pre style='color:#555'>> +    {
+        m_queue = std::move(list);
+        process_queue("relation", do_rels);
+
+        // Collect expiry tree information from all clones and merge it back
+        // into the original outputs.
+        for (auto const &clone : m_clones) {
+            auto it = clone.begin();
+            for (auto const &output : m_outputs) {
+                assert(it != clone.end());
+                if (*it) {
+                    output->merge_expire_trees(it->get());
+                }
+                ++it;
+            }
+        }
</pre>
<p>Can we put this for loop in its own function? It has nothing to do with the relation processing, it is just another step to be done when all processing is done.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/osm2pgsql/pull/1176#discussion_r428302588">src/osmdata.cpp</a>:</p>
<pre style='color:#555'>> @@ -297,40 +323,14 @@ struct pending_threaded_processor
         }
     }
 
-    void process_ways(idlist_t &&list)
-    {
-        m_queue = std::move(list);
</pre>
<p>There really is no need anymore that m_queue is a member.</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/1176#pullrequestreview-415683669">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AA6353VRCZKH2CGSDLVRSUTRSRAS5ANCNFSM4NFW5W5A">unsubscribe</a>.<img src="https://github.com/notifications/beacon/AA6353VAGUQ6M6MLGE7EJGLRSRAS5A5CNFSM4NFW5W5KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGODDDNIVI.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/1176#pullrequestreview-415683669",
"url": "https://github.com/openstreetmap/osm2pgsql/pull/1176#pullrequestreview-415683669",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>