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.

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

Use if constexpr macro instead of condition macro. #1247

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

awulkiew
Copy link
Member

This PR replaces uses of BOOST_GEOMETRY_CONDITION with BOOST_GEOMETRY_CONSTEXPR if possible and it is not destructive to readability.

Unfortunately it as less straightforward than I thought it would be because with MSVC and level 4 warnings enabled "unreachable code" warning is generated of some code exists outside if constexpr block. In such case an additional else block has to be added to prevent the warning. This means there are more blocks and indentations.

Constexpr conditions has to be checked separately from regular conditions. This means that some of the code has to be repeated e.g. if it has to be executed for both compile-time or run-time condition. This also can add more blocks and indentations.

For the reasons above I left BOOST_GEOMETRY_CONDITION in formulas because code like this:

if (Foo == 0)
{
   return bar;
}
// update bar
if (Foo == 1)
{
   return bar;
}
// update bar
etc.

would force me to add else brackets. Unless we agreed to write if constexpr differently than "regular" ifs, with different brackets placement, indentations, etc. I can't think of any way that would look good.

So if you think that some of the changes look bad feel free to point it out so I'll revert it to BOOST_GEOMETRY_CONDITION.

@@ -1,10 +1,10 @@
// Boost.Geometry (aka GGL, Generic Geometry Library)

// Copyright (c) 2007-2012 Barend Gehrels, Amsterdam, the Netherlands.
Copy link
Collaborator

@barendgehrels barendgehrels Feb 25, 2024

Choose a reason for hiding this comment

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

-> 2024

(it's about the next line, but you can ignore this comment)

@@ -457,7 +457,10 @@ RealNumber incircle(std::array<RealNumber, 2> const& p1,
RealNumber det = A_13 * (A_21_x_A_32[0] - A_31_x_A_22[0])
+ A_23 * (A_31_x_A_12[0] - A_11_x_A_32[0])
+ A_33 * (A_11_x_A_22[0] - A_21_x_A_12[0]);
if(Robustness == 0) return det;
if (BOOST_GEOMETRY_CONDITION(Robustness == 0))
Copy link
Collaborator

@barendgehrels barendgehrels Feb 25, 2024

Choose a reason for hiding this comment

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

Can this (and similar) made CONSTEXPR as well? Apart from needing the else branch, but you did that elsewhere too
Scratch that, it was already in the description of the PR

Copy link
Collaborator

@barendgehrels barendgehrels left a comment

Choose a reason for hiding this comment

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

I'm OK with this change.

I made a remark about the copyright (sometimes 2023, sometimes 2024) but it's not important, and maybe you started it already last year.

@barendgehrels
Copy link
Collaborator

Thanks for the change

@awulkiew
Copy link
Member Author

I made a remark about the copyright (sometimes 2023, sometimes 2024) but it's not important, and maybe you started it already last year.

Yes, I made the majority of changes last year and came back to it now. I was forced to resolve some conflicts so I updated the dates accordingly.

{
if ( BOOST_GEOMETRY_CONDITION(is_version_touches) )
bool output_spike = false;
Copy link
Member

Choose a reason for hiding this comment

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

this is also change the behavior e.g. the block

            tp.operations[0].operation = operation_intersection;
            tp.operations[1].operation = operation_blocked;
            *out++ = tp;
            //tp.operations[0].operation = operation_intersection;
            tp.operations[1].operation = operation_intersection;
            *out++ = tp;

was executed in all cases but now only in touch or append_colinnear_opposite cases.

Is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

If there is any change in behavior it was not intentional. But AFAIU there is no change here. We're talking about the block below at line 607(old) or 622(new) correct? In the old version it was executed:

if ( is_q_spike
  && ( BOOST_GEOMETRY_CONDITION(is_version_touches)
    || inters.d_info().arrival[1] == 1 ) )

now it's executed if output_spike is true and this is set in block:

if (is_q_spike)

and only

if BOOST_GEOMETRY_CONSTEXPR (is_version_touches)

or

else if (inters.d_info().arrival[1] == 1) // Version == append_collinear_opposite

For convenience here is the old version and the new version.

Or am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

OK, I was looking only at the changes and thought you were fixing something. It is clear that you just rewrote it, and it is actually simpler this way. So, everything is fine here.

Copy link
Member

@vissarion vissarion left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@@ -69,7 +69,7 @@ std::pair<bool, bool> point_multipoint_check(Point const& point,
// point_in_geometry could be used here but why iterate over MultiPoint twice?
// we must search for a point in the exterior because all points in MultiPoint can be equal


Copy link
Member

Choose a reason for hiding this comment

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

this is probably here by mistake.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants