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

clang-tidy: simplify some algorithms #14948

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

clang-tidy: simplify some algorithms #14948

wants to merge 2 commits into from

Conversation

neheb
Copy link
Contributor

@neheb neheb commented Dec 9, 2024

Just a cleanup to prepare for C++20

I have:

@coveralls
Copy link

coveralls commented Dec 9, 2024

Pull Request Test Coverage Report for Build 12303894321

Details

  • 4 of 5 (80.0%) changed or added relevant lines in 3 files are covered.
  • 98 unchanged lines in 10 files lost coverage.
  • Overall coverage increased (+11.0%) to 73.794%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pdns/arguments.cc 0 1 0.0%
Files with Coverage Reduction New Missed Lines %
pdns/recursordist/test-rec-tcounters_cc.cc 2 95.77%
pdns/recursordist/test-syncres_cc2.cc 3 88.91%
pdns/recursordist/rec-main.cc 3 62.38%
modules/godbcbackend/sodbc.cc 4 70.8%
pdns/packethandler.cc 5 72.44%
pdns/recursordist/syncres.cc 7 79.49%
pdns/recursordist/taskqueue.cc 11 34.15%
pdns/recursordist/test-syncres_cc1.cc 13 89.14%
modules/lmdbbackend/lmdbbackend.cc 16 72.5%
pdns/recursordist/rec-taskqueue.cc 34 40.14%
Totals Coverage Status
Change from base Build 12285701287: 11.0%
Covered Lines: 81221
Relevant Lines: 104054

💛 - Coveralls

@neheb neheb force-pushed the ll branch 5 times, most recently from e1dcdad to 4b20984 Compare December 10, 2024 02:43
@omoerbeek
Copy link
Member

I am a bit confused. Why prefer the boost variation of things that are available in standard C++? In general, I think the policy should be for features available both in boost and standard C++ to lean to the standard C++ variant.

@neheb
Copy link
Contributor Author

neheb commented Dec 10, 2024

std::ranges comes in C++20.

@omoerbeek
Copy link
Member

std::ranges comes in C++20.

I know that, but what's the reason to move to range::sort if the regular sort is fine?

@neheb
Copy link
Contributor Author

neheb commented Dec 10, 2024

Readability mostly.

@omoerbeek
Copy link
Member

I am not convinced. To me this PR looks quite a lot of churn with no real benefit.

@neheb
Copy link
Contributor Author

neheb commented Dec 10, 2024

Do note that clang-tidy makes the transformations away from boost with C++20

@omoerbeek
Copy link
Member

Do note that clang-tidy makes the transformations away from boost with C++20

Which is in line with the policy to use std c++ if possible. I'll let the other maintainers chime in to see what they think about this.

@rgacogne
Copy link
Member

Thank you for this PR, but I'm afraid I'm with Otto on this one: I have been trying to reduce the use of Boost in DNSdist whenever possible. I would love to be able to use C++20 but we are not quite there yet, and in the meantime I would prefer to use std C++ whenever possible.

@omoerbeek
Copy link
Member

Note existing calls to sort (without namespace) ad a few other functions use https://en.cppreference.com/w/cpp/language/adl to find the function. In the cases I checked, the arguments refer to std:: datatypes, so they resolve to using a function instd::.

@neheb neheb force-pushed the ll branch 2 times, most recently from d0ce8c8 to 8fced90 Compare December 10, 2024 23:50
@neheb
Copy link
Contributor Author

neheb commented Dec 10, 2024

OK. reduced boost usage and simplified some algorithms. What do you think?

@@ -760,7 +762,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck)
}

dnsdist::configuration::updateRuntimeConfiguration([&server](dnsdist::configuration::RuntimeConfiguration& config) {
config.d_backends.erase(std::remove(config.d_backends.begin(), config.d_backends.end(), server), config.d_backends.end());
boost::range::remove_erase(config.d_backends, server);
Copy link
Member

Choose a reason for hiding this comment

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

I would rather not introduce a boost::range for such a trivial change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

These are unused

Signed-off-by: Rosen Penev <[email protected]>
Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

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

Looks good to me now, thanks! I guess we should update the title of the PR, thought, because as it is right now it would lead to a misleading entry in the ChangeLog.

@neheb neheb changed the title use boost algorithms clang-tidy: simplify some algorithms Dec 13, 2024
@neheb
Copy link
Contributor Author

neheb commented Dec 13, 2024

Done.

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.

4 participants