Skip to content

refactor: Rename subprocess.hpp to follow our header name conventions #29910

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.

Sign up for GitHub

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

Jump to bottom
Merged
fanquake merged 2 commits into from
Apr 24, 2024
Merged

refactor: Rename subprocess.hpp to follow our header name conventions #29910

fanquake merged 2 commits into from
Apr 24, 2024

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Apr 19, 2024 •
edited
Loading

This PR renames the header *.hpp --> *.h and adjusts the header guard name, which makes it available for processing by linters.

Fixed the following linter warning:

The locale dependent function strerror(...) appears to be used:
src/util/subprocess.h:    std::runtime_error( err_msg + ": " + std::strerror(err_code) )

Unnecessary locale dependence can cause bugs that are very tricky to isolate and fix. Please avoid using locale-dependent functions if possible.

Advice not applicable in this specific case? Add an exception by updating the ignore list in /bitcoin/test/lint/lint-locale-dependence.py
^---- failure generated from lint-locale-dependence.py

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 19, 2024 •
edited
Loading

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.

Type Reviewers
ACK TheCharlatan
Concept ACK theStack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

@fanquake
Copy link
Member

fanquake commented Apr 19, 2024 •
edited
Loading

Why not put the windows change with the windows change: #29868 ?
Looks like the other change could just be PR'd on it's own, and doesn't need to be based on anything?
Avoiding conflicts doesn't seem necessary given this branch already conflicts as-is?

@hebasto hebasto changed the title Refactor subprocess.hpp to follow our conventions refactor: Rename subprocess.hpp to follow our header name conventions Apr 19, 2024
@hebasto hebasto marked this pull request as ready for review April 19, 2024 10:03
@hebasto
Copy link
Member Author

hebasto commented Apr 19, 2024

Why not put the windows change with the windows change: #29868 ? Looks like the other change could just be PR'd on it's own, and doesn't need to be based on anything? Avoiding conflicts doesn't seem necessary given this branch already conflicts as-is?

Thanks! Reworked per your suggestions.

@hebasto
Copy link
Member Author

hebasto commented Apr 19, 2024 •
edited
Loading

Drafted again until resolving a linter warning. (The header is processed by linters now).

@hebasto hebasto marked this pull request as draft April 19, 2024 10:06
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 19, 2024

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/24018238387

@hebasto
Copy link
Member Author

hebasto commented Apr 19, 2024

Drafted again until resolving a linter warning. (The header is processed by linters now).

Linter warnings have been resolved. But the PR remains as a draft because the #29865 will make the diff smaller.

This was referenced Apr 19, 2024
Merged
Merged
@hebasto hebasto mentioned this pull request Apr 23, 2024
Merged
@hebasto hebasto marked this pull request as ready for review April 23, 2024 17:25
@hebasto
Copy link
Member Author

hebasto commented Apr 23, 2024 •
edited
Loading

Rebased and undrafted.

@theStack
Copy link
Contributor

theStack commented Apr 23, 2024

Concept ACK

@sedited
Copy link
Contributor

sedited commented Apr 24, 2024

Resolved linter warnings.

Can you mention this in the PR description?

@hebasto
Copy link
Member Author

hebasto commented Apr 24, 2024

Resolved linter warnings.

Can you mention this in the PR description?

Thanks! The PR description has been updated.

sedited
sedited approved these changes Apr 24, 2024
View reviewed changes
Copy link
Contributor

@sedited sedited left a comment

Choose a reason for hiding this comment

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

ACK 08f756b

@DrahtBot DrahtBot requested a review from theStack April 24, 2024 14:22
@fanquake fanquake merged commit 50729c0 into bitcoin:master Apr 24, 2024
@hebasto hebasto deleted the 240419-sp-win32 branch April 24, 2024 16:30
@bitcoin bitcoin locked and limited conversation to collaborators Apr 24, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sedited sedited sedited approved these changes

@theStack theStack Awaiting requested review from theStack

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@hebasto @DrahtBot @fanquake @theStack @sedited