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

comparison differentiates absent and empty components #645

Merged
merged 1 commit into from
Dec 22, 2022

Conversation

alandefreitas
Copy link
Member

This PR simplifies the URL comparison function. It also fixes a bug where the difference between some absent and empty components was not considered. This fix was part of #425, which is now obsoleted.

@cppalliance-bot
Copy link

@codecov
Copy link

codecov bot commented Dec 22, 2022

Codecov Report

Merging #645 (3d783b6) into develop (cbcfa6a) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #645      +/-   ##
===========================================
- Coverage    96.48%   96.39%   -0.10%     
===========================================
  Files          138      138              
  Lines         6746     6789      +43     
===========================================
+ Hits          6509     6544      +35     
- Misses         237      245       +8     
Impacted Files Coverage Δ
include/boost/url/authority_view.hpp 93.93% <ø> (ø)
include/boost/url/impl/authority_view.ipp 85.09% <100.00%> (+4.93%) ⬆️
include/boost/url/impl/url_view_base.ipp 96.68% <100.00%> (-1.28%) ⬇️
include/boost/url/detail/impl/normalize.ipp 90.84% <0.00%> (-1.24%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbcfa6a...3d783b6. Read the comment docs.

@vinniefalco
Copy link
Member

Seems mostly ok except for style nits but what is the motivation for this?

@alandefreitas
Copy link
Member Author

Seems mostly ok except for style nits

Great. I'll fix that.

but what is the motivation for this?

The main motivation is fixing the bug. The previous function would compare an absent component and an empty component as the same thing, but that's wrong.

BOOST_TEST_EQ(url("https:"), url("https://"));
BOOST_TEST_EQ(url("https://@www.boost.org"), url("https://www.boost.org"));
BOOST_TEST_EQ(url("https:"), url("https:?"));
BOOST_TEST_EQ(url("https:"), url("https:#"));
// ... etc

should fail.

I'll improve coverage with a few more tests and the difference is going to be more explicit.

@vinniefalco
Copy link
Member

"simplify URL comparison" doesnt' really tell me that there's a bug in the current release of Boost.URL

@alandefreitas
Copy link
Member Author

"simplify URL comparison" doesnt' really tell me that there's a bug in the current release of Boost.URL

Yes. I chose that because it's not the only thing happening in the PR and because of the trend of "refactor xx" being a synonym for "fix xx".

@alandefreitas alandefreitas linked an issue Dec 22, 2022 that may be closed by this pull request
@alandefreitas alandefreitas changed the title simplify URL comparison comparison differentiates absent and empty components Dec 22, 2022
@cppalliance-bot
Copy link

@alandefreitas alandefreitas merged commit 50afdde into boostorg:develop Dec 22, 2022
@alandefreitas alandefreitas deleted the refactor_compare branch December 22, 2022 20:12
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.

False posititive in URL comparison
3 participants