Skip to content
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.

Sign up for GitHub

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

Closed
wants to merge 13 commits into from
Closed

Refactor: Replace parts of boost::optional with std::optional (#6551,… #6593

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
97d3fcb
Replace some boost::optional with std::optional
Apr 4, 2023
850a3a7
Updated guidance/
Apr 4, 2023
81d6e1f
Updated nodejs/
Apr 4, 2023
bb58c9d
Fix compiler error in include/util/range_table.hpp
Apr 9, 2023
f98d73c
Reformatted using clang-format-10
Apr 14, 2023
fb0400f
Switched to not using std::optional for query_heap.hpp
Apr 14, 2023
c9ac8ee
Comment fix
Apr 14, 2023
e920bbb
Resolve conflicts with #6602
Apr 15, 2023
ec74be3
Merge branch 'master' into boost_optional
mugr1x Apr 15, 2023
7f6900d
Reverted changes in query_heap.hpp
Apr 17, 2023
c42d326
Added block_counter variable to a preprocessor check, minor bug. (#6596)
AlTimofeyev Apr 16, 2023
9242365
Reverted one range_table.hpp fix
Apr 18, 2023
f54aa0a
Make GitHub and CI happy
Apr 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
- NodeJS:
- CHANGED: Use node-api instead of NAN. [#6452](https://github.com/Project-OSRM/osrm-backend/pull/6452)
- Misc:
- FIXED: Partial fix migration from boost::optional to std::optional [#6551](https://github.com/Project-OSRM/osrm-backend/issues/6551), see also [#6592](https://github.com/Project-OSRM/osrm-backend/issues/6592)
- CHANGED: Move vector in CSVFilesParser instead copying it. [#6470](https://github.com/Project-OSRM/osrm-backend/pull/6470)
- REMOVED: Get rid of unused functions in util/json_util.hpp. [#6446](https://github.com/Project-OSRM/osrm-backend/pull/6446)
- FIXED: Apply workaround for Conan installation issue on CI. [#6442](https://github.com/Project-OSRM/osrm-backend/pull/6442)
Expand Down
5 changes: 2 additions & 3 deletions include/guidance/intersection_handler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,10 @@

#include <algorithm>
#include <cstddef>
#include <optional>
#include <utility>
#include <vector>

#include <boost/optional.hpp>

namespace osrm::guidance
{

Expand Down Expand Up @@ -129,7 +128,7 @@ class IntersectionHandler
// ^ via
//
// For this scenario returns intersection at `b` and `b`.
boost::optional<IntersectionHandler::IntersectionViewAndNode>
std::optional<IntersectionHandler::IntersectionViewAndNode>
getNextIntersection(const NodeID at, const EdgeID via) const;

bool isSameName(const EdgeID source_edge_id, const EdgeID target_edge_id) const;
Expand Down
9 changes: 4 additions & 5 deletions include/guidance/sliproad_handler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,9 @@

#include "util/node_based_graph.hpp"

#include <optional>
#include <vector>

#include <boost/optional.hpp>

namespace osrm::guidance
{

Expand Down Expand Up @@ -43,9 +42,9 @@ class SliproadHandler final : public IntersectionHandler
Intersection intersection) const override final;

private:
boost::optional<std::size_t> getObviousIndexWithSliproads(const EdgeID from,
const Intersection &intersection,
const NodeID at) const;
std::optional<std::size_t> getObviousIndexWithSliproads(const EdgeID from,
const Intersection &intersection,
const NodeID at) const;

// Next intersection from `start` onto `onto` is too far away for a Siproad scenario
bool nextIntersectionIsTooFarAway(const NodeID start, const EdgeID onto) const;
Expand Down
7 changes: 3 additions & 4 deletions include/guidance/turn_handler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,8 @@
#include "util/attributes.hpp"
#include "util/node_based_graph.hpp"

#include <boost/optional.hpp>

#include <cstddef>
#include <optional>
#include <utility>
#include <vector>

Expand Down Expand Up @@ -72,7 +71,7 @@ class TurnHandler final : public IntersectionHandler

bool hasObvious(const EdgeID &via_edge, const Fork &fork) const;

boost::optional<Fork> findForkCandidatesByGeometry(Intersection &intersection) const;
std::optional<Fork> findForkCandidatesByGeometry(Intersection &intersection) const;

bool isCompatibleByRoadClass(const Intersection &intersection, const Fork fork) const;

Expand All @@ -96,7 +95,7 @@ class TurnHandler final : public IntersectionHandler
handleDistinctConflict(const EdgeID via_edge, ConnectedRoad &left, ConnectedRoad &right) const;

// Classification
boost::optional<Fork> findFork(const EdgeID via_edge, Intersection &intersection) const;
std::optional<Fork> findFork(const EdgeID via_edge, Intersection &intersection) const;

OSRM_ATTR_WARN_UNUSED
Intersection assignLeftTurns(const EdgeID via_edge,
Expand Down
8 changes: 4 additions & 4 deletions include/nodejs/node_osrm_support.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@
#include <napi.h>

#include <boost/assert.hpp>
#include <boost/optional.hpp>

#include <algorithm>
#include <iostream>
#include <iterator>
#include <optional>
#include <sstream>
#include <stdexcept>
#include <string>
Expand Down Expand Up @@ -345,11 +345,11 @@ inline engine_config_ptr argumentsToEngineConfig(const Napi::CallbackInfo &args)
return engine_config;
}

inline boost::optional<std::vector<osrm::Coordinate>>
inline std::optional<std::vector<osrm::Coordinate>>
parseCoordinateArray(const Napi::Array &coordinates_array)
{
Napi::HandleScope scope(coordinates_array.Env());
boost::optional<std::vector<osrm::Coordinate>> resulting_coordinates;
std::optional<std::vector<osrm::Coordinate>> resulting_coordinates;
std::vector<osrm::Coordinate> temp_coordinates;

for (uint32_t i = 0; i < coordinates_array.Length(); ++i)
Expand Down Expand Up @@ -968,7 +968,7 @@ inline bool parseCommonParameters(const Napi::Object &obj, ParamType &params)

inline PluginParameters argumentsToPluginParameters(
const Napi::CallbackInfo &args,
const boost::optional<osrm::engine::api::BaseParameters::OutputFormatType> &output_format = {})
const std::optional<osrm::engine::api::BaseParameters::OutputFormatType> &output_format = {})
{
if (args.Length() < 3 || !args[1].IsObject())
{
Expand Down
8 changes: 4 additions & 4 deletions include/util/coordinate_calculation.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@
#include "util/coordinate.hpp"

#include <boost/math/constants/constants.hpp>
#include <boost/optional.hpp>

#include <algorithm>
#include <cmath>
#include <numeric>
#include <optional>
#include <utility>
#include <vector>

Expand Down Expand Up @@ -102,9 +102,9 @@ double bearing(const Coordinate first_coordinate, const Coordinate second_coordi
double computeAngle(const Coordinate first, const Coordinate second, const Coordinate third);

// find the center of a circle through three coordinates
boost::optional<Coordinate> circleCenter(const Coordinate first_coordinate,
const Coordinate second_coordinate,
const Coordinate third_coordinate);
std::optional<Coordinate> circleCenter(const Coordinate first_coordinate,
const Coordinate second_coordinate,
const Coordinate third_coordinate);

// find the radius of a circle through three coordinates
double circleRadius(const Coordinate first_coordinate,
Expand Down
11 changes: 5 additions & 6 deletions include/util/geojson_debug_policies.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#ifndef OSRM_GEOJSON_DEBUG_POLICIES
#define OSRM_GEOJSON_DEBUG_POLICIES

#include <optional>
#include <vector>

#include "extractor/query_node.hpp"
Expand All @@ -9,8 +10,6 @@
#include "util/node_based_graph.hpp"
#include "util/typedefs.hpp"

#include <boost/optional.hpp>

namespace osrm::util
{

Expand All @@ -20,7 +19,7 @@ struct NodeIdVectorToLineString

// converts a vector of node ids into a linestring geojson feature
util::json::Object operator()(const std::vector<NodeID> &node_ids,
const boost::optional<json::Object> &properties = {}) const;
const std::optional<json::Object> &properties = {}) const;

const std::vector<util::Coordinate> &node_coordinates;
};
Expand All @@ -29,7 +28,7 @@ struct CoordinateVectorToLineString
{
// converts a vector of node ids into a linestring geojson feature
util::json::Object operator()(const std::vector<util::Coordinate> &coordinates,
const boost::optional<json::Object> &properties = {}) const;
const std::optional<json::Object> &properties = {}) const;
};

struct NodeIdVectorToMultiPoint
Expand All @@ -38,7 +37,7 @@ struct NodeIdVectorToMultiPoint

// converts a vector of node ids into a linestring geojson feature
util::json::Object operator()(const std::vector<NodeID> &node_ids,
const boost::optional<json::Object> &properties = {}) const;
const std::optional<json::Object> &properties = {}) const;

const std::vector<util::Coordinate> &node_coordinates;
};
Expand All @@ -47,7 +46,7 @@ struct CoordinateVectorToMultiPoint
{
// converts a vector of node ids into a linestring geojson feature
util::json::Object operator()(const std::vector<util::Coordinate> &coordinates,
const boost::optional<json::Object> &properties = {}) const;
const std::optional<json::Object> &properties = {}) const;
};

} // namespace osrm::util
Expand Down
5 changes: 2 additions & 3 deletions include/util/geojson_debug_policy_toolkit.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@

#include <algorithm>
#include <iterator>

#include <boost/optional.hpp>
#include <optional>

namespace osrm::util
{
Expand Down Expand Up @@ -84,7 +83,7 @@ struct NodeIdToCoordinate

inline util::json::Object makeFeature(std::string type,
util::json::Array coordinates,
const boost::optional<util::json::Object> &properties = {})
const std::optional<util::json::Object> &properties = {})
{
util::json::Object result;
result.values["type"] = "Feature";
Expand Down
18 changes: 9 additions & 9 deletions include/util/query_heap.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand Down Expand Up @@ -289,26 +289,26 @@ class QueryHeap
return inserted_nodes[index].node == node;
}

boost::optional<HeapNode &> GetHeapNodeIfWasInserted(const NodeID node)
HeapNode *GetHeapNodeIfWasInserted(const NodeID node)
{
const auto index = node_index.peek_index(node);
if (index >= static_cast<decltype(index)>(inserted_nodes.size()) ||
inserted_nodes[index].node != node)
{
return {};
return nullptr;
}
return inserted_nodes[index];
return &inserted_nodes[index];
}

boost::optional<const HeapNode &> GetHeapNodeIfWasInserted(const NodeID node) const
const HeapNode *GetHeapNodeIfWasInserted(const NodeID node) const
{
const auto index = node_index.peek_index(node);
if (index >= static_cast<decltype(index)>(inserted_nodes.size()) ||
inserted_nodes[index].node != node)
{
return {};
return 0;
}
return inserted_nodes[index];
return &inserted_nodes[index];
}

NodeID Min() const
Expand Down Expand Up @@ -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));
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my previous link posted above:
https://www.boost.org/doc/libs/1_76_0/doc/html/boost/heap/d_ary_heap.html#id-1_3_18_6_2_1_1_1_22_17-bb
My thoughts behind this are that there is logic in multiple files e.g. include/engine/routing_algorithms/routing_base_mld.hpp that checks for a certain "to_weight" to be smaller than the current "toHeapNode->weight" and then calls DecreaseKey which in turn calls heap.increase with "to_weight".
So the docs say: "The new value is expected to be less than the current one" for decrease not increase.
Also I guess it kinda fixed a segfault for me while testing. (I know for sure it was this one, however might only have emerged with now reverted changes since now not using std::optional instead just pointers)
Am I completely mistaken here ? I still get from A to B though using my setup e.g. with the Germany map.

}

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:
Expand Down
4 changes: 3 additions & 1 deletion include/util/range_table.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

@danpat danpat Apr 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

@AlTimofeyev AlTimofeyev Apr 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The source code on master branch has block_counter = 0 on line 85, meanwhile in this commit it is placed on line 84. Could that be what's causing CI to fail?

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlTimofeyev Well yes. Idk that much about CI.
@SiarheiFedartsou I did so #6611. Should I close this one ?
I also tried to rewrite some parsers. Point here is that the ones that are easy to rewrite work fine with switching to std::optional but the /server core API (and /engine because it there are crosslinks) don't.
I think this would be considered a too breaking change (at least ~1k lines of code) of new parser-related code.
Do you think there is value in #6592 or should I close this too ?
Also off-topic, but did you receive my email regarding GSoC. I was asking for what you mentors would expect. (I submitted my second version a bit late, though in time. Hope that is not a problem.)

unsigned block_sum = 0;
#endif
for (const unsigned l : lengths)
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions include/util/timezones.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@
#include <boost/filesystem/path.hpp>
#include <boost/geometry.hpp>
#include <boost/geometry/index/rtree.hpp>
#include <boost/optional.hpp>

#include <rapidjson/document.h>

#include <chrono>
#include <optional>

namespace osrm::updater
{
Expand All @@ -34,7 +34,7 @@ class Timezoner
Timezoner(const char geojson[], std::time_t utc_time_now);
Timezoner(const boost::filesystem::path &tz_shapes_filename, std::time_t utc_time_now);

boost::optional<struct tm> operator()(const point_t &point) const;
std::optional<struct tm> operator()(const point_t &point) const;

private:
void LoadLocalTimesRTree(rapidjson::Document &geojson, std::time_t utc_time);
Expand Down
9 changes: 4 additions & 5 deletions src/guidance/intersection_handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,7 @@ void IntersectionHandler::assignTrivialTurns(const EdgeID via_eid,
}
}

boost::optional<IntersectionHandler::IntersectionViewAndNode>
std::optional<IntersectionHandler::IntersectionViewAndNode>
IntersectionHandler::getNextIntersection(const NodeID at, const EdgeID via) const
{
// We use the intersection generator to jump over traffic signals, barriers. The intersection
Expand All @@ -449,7 +449,7 @@ IntersectionHandler::getNextIntersection(const NodeID at, const EdgeID via) cons
if (intersection_parameters.node == SPECIAL_NODEID ||
intersection_parameters.edge == SPECIAL_EDGEID)
{
return boost::none;
return {};
}

auto intersection = extractor::intersection::getConnectedRoads<false>(node_based_graph,
Expand All @@ -464,11 +464,10 @@ IntersectionHandler::getNextIntersection(const NodeID at, const EdgeID via) cons

if (intersection.size() <= 2 || intersection.isTrafficSignalOrBarrier())
{
return boost::none;
return {};
}

return boost::make_optional(
IntersectionViewAndNode{std::move(intersection), intersection_node});
return std::make_optional(IntersectionViewAndNode{std::move(intersection), intersection_node});
}

bool IntersectionHandler::isSameName(const EdgeID source_edge_id, const EdgeID target_edge_id) const
Expand Down
Loading