Replace several Boost headers with C++11/C++14 STL alternatives.#1425
Replace several Boost headers with C++11/C++14 STL alternatives.#1425tinko92 wants to merge 1 commit intoboostorg:developfrom
Conversation
tinko92
force-pushed
the
chore/replace-boost-dependencies-with-stl
branch
from
3109143 to
a6d17ec
Compare
| template <typename Section> | ||
| inline bool apply(Section const& section1, Section const& section2, | ||
| bool first = true) | ||
| bool = true) |
There was a problem hiding this comment.
So we can remove it. I'm fine with it like this in this PR, we can change it in a follow up
| < | ||
| Areal1, Areal2, single_out, | ||
| point_tag, linestring_tag, polygon_tag | ||
| >); |
There was a problem hiding this comment.
This I don't like so much. ignore_unused was meant exactly to avoid this cryptic statement.
Is it solveable in another way?
(Why is the whole statement there anyway - but that is another question...)
There was a problem hiding this comment.
I will test taking a pointer of this type and assign it to ignore. I will also try to check why this statement was there, maybe it can be dropped.
| static bool constexpr use_start_turn = true; | ||
| static bool constexpr use_handle_as_touch = true; | ||
| static bool constexpr use_handle_as_equal = true; | ||
| static bool constexpr use_handle_imperfect_touch = true; |
There was a problem hiding this comment.
Is static constexpr valid? I didn't check but I remembered there was something with it.
Why is it actually changed? (A big PR can better have exactly one subject)
There was a problem hiding this comment.
But anyway, if it is valid, it is fine.
Thanks for the PR, it seems a huge effort and a huge style improvement.
There was a problem hiding this comment.
I think, it changes the meaning in so far as that these values will not have actual storage, which I think is appropriate in this context, since I understood that these are never used in ways that requires taking their addresses but rather for decisions that can all be resolved at compile time. But I can just take it out of this PR tomorrow to have a more clear scope.
| ip_info const& get() const | ||
| { | ||
| BOOST_STATIC_ASSERT(I < 2); | ||
| static_assert(I < 2, "I must not be greater than 1"); |
There was a problem hiding this comment.
This sounds like a funny sentence ;-)
There was a problem hiding this comment.
I chose a message like "I is out of bound" elsewhere, I think for the matrix extension. I should probably harmonize it in some way. Would "out of bound" be a fitting descriptor for any case where it e.g. exceeds the dimension?
There was a problem hiding this comment.
It is no problem, it just sounds like I (me) should stay small ;-)
You can keep it.
barendgehrels
left a comment
barendgehrels
left a comment
There was a problem hiding this comment.
I'm basically fine. Thanks a lot.
Some suggestions:
- split changes in
extensionsand other code, such that cherry-picking formasteris more convenient. @vissarion to comment if this is still convenient, or it is done in another way - make it smaller. It was called
boost alternativesbut there are also other changes such as changingtypedeftousing. That is indeed tempting - but it makes the scope larger - I didn't review every line but more than half of it and it all makes sense to me (one question there)
|
Thanks for the very quick review! 🙏 I will update the PR to address the points you mentioned. Edit: Some unexpected time constraints came up, I'll try to update the PR by the end of next week. |
I didn't mean you should revert the But splitting out extensions would still be convenient, I think. |
|
@tinko92 do you agree the extensions should get into a separate commit? Is that possible? |
|
@barendgehrels Yes, I agree. Sorry for not having updated this in a while. I'll update this based on the latest develop state as soon as I find time and seperate it into two commits. |
vissarion
left a comment
vissarion
left a comment
There was a problem hiding this comment.
Looks good to me! Nice modernization, Tinko, thanks! I am OK if the code splits in two commits (to separate extensions code).
3 participants
This PR removes a number of includes of Boost headers that are no longer required with C++14 because there are either language constructs (e.g.
static_assert) or standard library headers that fullfil similar functions (e.g.std::ignoreinstead ofboost::ignore_unused).Where it wasn't needed code was removed entirely (e.g.
boost::ignore_unusedwhen the variable in question was passed to another function already). Where it seemed approriate, it was replaced with more explicit constructs (e.g.static_assertsonstd::is_convertiblein various concept headers rather than assignments to unused variables making it closer to the patterns established in C++ concepts).Some modernisations/cleanups are included that I noticed along the way (e.g. the usual
typedef->using,some_type* some_name = 0;->some_type* some_name = nullptr, removal ofdistance_projected_point_ax.hpp, which was deprecated earlier).Because
std::ignoreis included it via<tuple>rather than<utility>so that it is compatible with older versions of the standard library (e.g. with GCC 11). This does not limit its intended usage with C++14 from today's perspective.No non-trivial refactoring was done in vararray. This type should maybe be replaced with
static_vector(inplace_vectorin C++26), which seems to be based on it and duplicates its functionality in Boost.Container. This was not included in this PR because it led to a performance regression.Some dependencies on Boost libraries now persist only for adapted geometry types. These were not touched because it would have changed documented behaviour. But IHMO it would be worth discussing whether these adapted geometries (Boost.Array and Boost.Tuple now, Boost.Range when the standard is raised to C++20 and Boost.Fusion when it is raised to C++26) could be deprecated for future removal, given that they are defined for types that are obsolete in C++14 which is the required minimum for Boost.Geometry.