refactor in DescriptorImpl::{IsSolvable, IsRange}

[lightyear15]

minor refactor commit that replaces some explicit for loops with functionally equivalent but more concise std::algorithms

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

[darosior]

Code looks correct to me, but in general i think we should avoid needless refactorings. Making the code more testable or less footguny for instance would be a good justification for a refactoring. But i don't think modernizing the implementation is one.

(I'm not opposed to merging this though, just pointing out it may not be the best use of everyone's time.)

[maflcko]

Not sure. If this is changed, why not use C++20 ranges algorithms, which would be even more concise?

However, I wonder what the point is of randomly addressing a single clang-tidy refactor hint in a single source location. If there is any value in a specific clang-tidy check, it should be enabled and changed for all code in one go. If there is little value or not enough value to enable it for all the code and enforce it in CI, then no code should be changed at all, no?

[lightyear15]

Not sure. If this is changed, why not use C++20 ranges algorithms, which would be even more concise?

However, I wonder what the point is of randomly addressing a single clang-tidy refactor hint in a single source location. If there is any value in a specific clang-tidy check, it should be enabled and changed for all code in one go. If there is little value or not enough value to enable it for all the code and enforce it in CI, then no code should be changed at all, no?

c++20 is disabled by default, so I don't think it's a good idea to use language/library features from that standard version.

Regarding the change itself, it's not coming from a clang-tidy warning or anything, it's just me, surfing the code and noticing a bit of room for improvement.

Feel free to reject it if you think it's not worth it.

[maflcko]

Regarding the change itself, it's not coming from a clang-tidy warning or anything

Fair enough, I presume it would be surfaced from https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/modernize/loop-convert.html . So I'd say we either use clang-tidy and enforce it, or not do it at all.

c++20 is disabled by default, so I don't think it's a good idea to use language/library features from that standard version.

I think this change here could wait until C++20 is enabled, see #28349 . There should be no rush to do change anything here (potentially to C++17, and then later again, to C++20)

[pablomartin4btc]

Thanks @lightyear15 for taking the time to find ways to improve the code.

I agree with both @darosior and @maflcko, even I like the new style of the proposed change, and I'm not against of having this merged if other reviewers find good reasons to do so, we could wait for std::ranges on C++ 20 (if we wanted to use that instead) and keep the PR as draft.

[maflcko]

I can't convert this to a draft, so I am closing for now. Let us know if you want this reopened after the C++20 switch.

[maflcko]

In the meantime, if you are looking for other good first issues:

• Search through the good first issues or the ones that are up for grabs. Some of them might no longer be applicable. So if you are interested, but unsure, you might want to leave a comment on the issue first.
• Write tests to improve the coverage. Any kind of test is welcome and coverage information can be obtained from a relatively recent coverage report. If you are unsure, don't hesitate to check back first.
• Help with review and testing. There are easy ones such as the gui and rpc. However, review on any open pull request is welcome. Review will also help you understand the codebase better.
• Help on meta projects related to Bitcoin Core, such as a high-level performance monitor.
• Join us on irc and let us know what you are interested in.