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

Fix operator overload on RouteParameters #6646

Merged
merged 4 commits into from
Aug 19, 2023

Conversation

whytro
Copy link
Contributor

@whytro whytro commented Jul 4, 2023

Issue

This PR targets an error in the operator overload in RouteParameters, which made the |= operator nonfunctional.

Tasklist

Requirements / Relations

There are no requirements or relations relevant to this PR.

inline RouteParameters::AnnotationsType operator|=(RouteParameters::AnnotationsType lhs,
RouteParameters::AnnotationsType rhs)
inline RouteParameters::AnnotationsType &operator|=(RouteParameters::AnnotationsType &lhs,
RouteParameters::AnnotationsType rhs)
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth deleting this function instead. I assume given the bug that it's not in use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can commit a change to remove it instead if it's preferred, but I think that given the use of the | operator, it might be more helpful to keep around fixed, since it can help when calculating the AnnotationsType (ie. able to do lhs |= rhs rather than lhs = lhs | rhs).

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, thanks for spotting 👍

Copy link
Member

@mjjbell mjjbell left a comment

Choose a reason for hiding this comment

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

LGTM.
@whytro Can you run npm audit fix --production on your branch and commit the changes?
That will fix CI.

@mjjbell mjjbell merged commit 3f9347c into Project-OSRM:master Aug 19, 2023
@whytro whytro deleted the routeparameter_operator branch August 20, 2023 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants