-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor: Replace parts of boost::optional with std::optional (#6551,… #6593
Changes from 5 commits
97d3fcb
850a3a7
81d6e1f
bb58c9d
f98d73c
fb0400f
c9ac8ee
e920bbb
ec74be3
7f6900d
c42d326
9242365
f54aa0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,11 +3,11 @@ | |
|
||
#include <boost/assert.hpp> | ||
#include <boost/heap/d_ary_heap.hpp> | ||
#include <boost/optional.hpp> | ||
|
||
#include <algorithm> | ||
#include <limits> | ||
#include <map> | ||
#include <optional> | ||
#include <unordered_map> | ||
#include <vector> | ||
|
||
|
@@ -289,7 +289,7 @@ class QueryHeap | |
return inserted_nodes[index].node == node; | ||
} | ||
|
||
boost::optional<HeapNode &> GetHeapNodeIfWasInserted(const NodeID node) | ||
std::optional<HeapNode> GetHeapNodeIfWasInserted(NodeID node) | ||
{ | ||
const auto index = node_index.peek_index(node); | ||
if (index >= static_cast<decltype(index)>(inserted_nodes.size()) || | ||
|
@@ -300,7 +300,7 @@ class QueryHeap | |
return inserted_nodes[index]; | ||
} | ||
|
||
boost::optional<const HeapNode &> GetHeapNodeIfWasInserted(const NodeID node) const | ||
std::optional<HeapNode> GetHeapNodeIfWasInserted(const NodeID node) const | ||
{ | ||
const auto index = node_index.peek_index(node); | ||
if (index >= static_cast<decltype(index)>(inserted_nodes.size()) || | ||
|
@@ -356,13 +356,13 @@ class QueryHeap | |
const auto index = node_index.peek_index(node); | ||
auto &reference = inserted_nodes[index]; | ||
reference.weight = weight; | ||
heap.increase(reference.handle, std::make_pair(weight, index)); | ||
heap.decrease(reference.handle, std::make_pair(weight, index)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is pretty fundamental to how the whole routing algorithm works. Can you explain exactly why this change is being made here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my previous link posted above: |
||
} | ||
|
||
void DecreaseKey(const HeapNode &heapNode) | ||
{ | ||
BOOST_ASSERT(!WasRemoved(heapNode.node)); | ||
heap.increase(heapNode.handle, std::make_pair(heapNode.weight, (*heapNode.handle).second)); | ||
heap.decrease(heapNode.handle, std::make_pair(heapNode.weight, (*heapNode.handle).second)); | ||
} | ||
|
||
private: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,9 +79,9 @@ template <unsigned BLOCK_SIZE, storage::Ownership Ownership> class RangeTable | |
unsigned last_length = 0; | ||
unsigned lengths_prefix_sum = 0; | ||
unsigned block_idx = 0; | ||
unsigned block_counter = 0; | ||
BlockT block; | ||
#ifndef BOOST_ASSERT_IS_VOID | ||
unsigned block_counter = 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These changes were merged to master in another PR #6596 - they should not also be showing up in this PR. I suspect some merges have been incorrectly applied or lost on the branch. This is probably also why CI is failing. I suspect if you tidy this up, CI should pass. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The source code on master branch has There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mugr1x may be try to squash all your commits into single one? Or open another PR? Tbh for me it is not clear why it is happening - looks more like some GH bug. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @AlTimofeyev Well yes. Idk that much about CI. |
||
unsigned block_sum = 0; | ||
#endif | ||
for (const unsigned l : lengths) | ||
|
@@ -109,7 +109,9 @@ template <unsigned BLOCK_SIZE, storage::Ownership Ownership> class RangeTable | |
if (BLOCK_SIZE == block_idx) | ||
{ | ||
diff_blocks.push_back(block); | ||
#ifndef BOOST_ASSERT_IS_VOID | ||
block_counter++; | ||
#endif | ||
} | ||
|
||
// we can only store strings with length 255 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I.e. now on each call we will copy HeapNode, right? How do we know that it won’t impact performance? May be worth refactoring it to
HeapNode*
instead(I mean use just pointer instead of optional here)? From some perspective optional<T&> is just overcomplicated way to express simple pointer…WDYT?
P.S. I didn’t have a time yet to look over the rest of PR, just something important what I noticed…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More than it: here you can see an example when we change contents of node in a heap and that by reference…
osrm-backend/include/engine/routing_algorithms/routing_base_mld.hpp
Line 331 in d516314
I.e. if we return it not by reference now most likely existing logic is broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did run my build and it did at least not segfault or got killed by kernel for too much RAM.
To cite from cppreference.com:
"There are no optional references; a program is ill-formed if it instantiates an optional with a reference type."
Search engine would suggest std::reference_wrapper for this task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the same time as I can see if we just change it to simple pointer instead of optional(I.e. won’t use neither boost::optional nor std::optional) the rest of code which uses this QueryHeap would “just work” without changes we had to make there…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the other comment std::optional defines an operator->. However in some cases you probably would need to change a deref etc. I'm just testing solely using pointers for these parts. Does not work out of the box, but the idea should be doable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boost::optional<T&>
seems to be effectively the same asT*
, isn’t it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is. Still forking the main branch I need replace some of the idioms. Do you prefer nullptr or some void coercion ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is void coercion? You mean
if (ptr == nullptr)
vsif (!ptr)
? Personally I like first option as more explicit, but I don’t think it would be blocker to merge this PR :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it's dreadfully simple, memory safety should be fine since it tests. I got too fed up with the nullptr version.
I think you need to approve CI again.