Fix Visual Studio warnings #1160
Conversation
awulkiew
commented
Jun 7, 2023
awulkiew
commented
- C4100 Unreferenced formal parameter
- C4127 Conditional expression is constant
- C4456 Declaration hides previous local declaration
- C4701 Potentially uninitialized local variable used
- C4702 Unreachable code
|
|
||
| if ( OverlayType != overlay_difference | ||
| && boost::size(multipoint1) > boost::size(multipoint2) ) | ||
| if (BOOST_GEOMETRY_CONDITION( |
There was a problem hiding this comment.
What do you think about adding a macro allowing to conditionally use C++17 if constexpr?
There was a problem hiding this comment.
It can probably be integrated into BOOST_GEOMETRY_CONDITION ? Such that we don't need another one?
There was a problem hiding this comment.
@barendgehrels Unfortunately we do need two separate macros. AFAIR there are rare places where a condition can be constant or not, e.g. some policy can define a variable as static or dynamic member. Of course we could agree to drop static members in such cases.
Another thing is that I think we should consider something which looks more like the language feature. I'd also prefer to keep if so to not include it in the macro in order to allow the editor to highlight the key word to make it more readable. So maybe something like this:
if constexpr (OverlayType != overlay_difference)
vs
if BOOST_GEOMETRY_CONSTEXPR (OverlayType != overlay_difference)
There was a problem hiding this comment.
See the condition below in pj_transform.hpp line 255:
if( BOOST_GEOMETRY_CONDITION(srcdefn.vto_meter != 1.0 && dimension > 2) )
where srcdefn.vto_meter may or may not be static but dimension is always static. In this case if constexpr should be used for dimension and BOOST_GEOMETRY_CONDITION for srcdefn.vto_meter. Or vto_meter should always be non-static member, also in static projections .
There was a problem hiding this comment.
OK, I see, and saw it in your new PR this morning. Cool.
| counter& the_state, | ||
| Ring const& full_ring) | ||
| counter& the_state) |
There was a problem hiding this comment.
@barendgehrels Is that ok or should I rather leave the parameter and remove the name?
There was a problem hiding this comment.
It's OK, thanks. I also answered elsewhere before seeing this comment.
|
|
||
| if ( OverlayType != overlay_difference | ||
| && boost::size(multipoint1) > boost::size(multipoint2) ) | ||
| if (BOOST_GEOMETRY_CONDITION( |
| geometry::strategy::buffer::place_on_ring_type place_on_ring, State& state) const | ||
| { | ||
| return strategy.apply(point, p1, p2, place_on_ring, m_is_convex, state, get_full_ring()); | ||
| return strategy.apply(point, p1, p2, place_on_ring, m_is_convex, state); |
There was a problem hiding this comment.
Hmm, this was probably a left over, only used for debugging.
Deletion looks good to me, thanks.
| auto rot_end = boost::end(ring); | ||
| std::size_t rot_index = index; | ||
| if (is_closed_in && size > 1) | ||
| if (BOOST_GEOMETRY_CONDITION(is_closed_in && size > 1)) |
There was a problem hiding this comment.
Related to if constexpr, isn't it, in cases like this, better to only apply the const part on the part being const?
I mean: if (BOOST_GEOMETRY_CONDITION(is_closed_in) && size > 1) ?
|
|
||
| // Open output if needed | ||
| if (! is_closed_out && boost::size(out) > 1) | ||
| if (BOOST_GEOMETRY_CONDITION(! is_closed_out && boost::size(out) > 1)) |
| } | ||
|
|
||
| if (LongSegment && lat1r != lat2r) // not for segments parallel to equator | ||
| if (BOOST_GEOMETRY_CONDITION(LongSegment && lat1r != lat2r)) // not for segments parallel to equator |
There was a problem hiding this comment.
And here, and more.
barendgehrels
left a comment
barendgehrels
left a comment
There was a problem hiding this comment.
Looks good to me in general, the only thing is that I would like to separate the parts really being const from the parts that might vary.
- C4100 Unreferenced formal parameter - C4127 Conditional expression is constant - C4456 Declaration hides previous local declaration - C4701 Potentially uninitialized local variable used - C4702 Unreachable code
|
Thanks. I separated them. |
3 participants
