Skip to content

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Jun 9, 2022

These files change infrequently, and not much header shuffling is required.

We don't add everything in src/util/ yet, because IWYU makes some
dubious suggestions, which I'm going to follow up with upstream.

Soon we'll swap src/util/xyz.cpp for just src/util/.

@hebasto
Copy link
Member

hebasto commented Jun 9, 2022

Concept ACK.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 9, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25112 (util: Move error message formatting of NonFatalCheckError to cpp by MarcoFalke)
  • #24058 (BIP-322 basic support by kallewoof)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Comment on lines +49 to +61
" src/util/bip32.cpp"\
" src/util/bytevectorhash.cpp"\
" src/util/error.cpp"\
" src/util/getuniquepath.cpp"\
" src/util/hasher.cpp"\
" src/util/message.cpp"\
" src/util/moneystr.cpp"\
" src/util/serfloat.cpp"\
" src/util/spanparsing.cpp"\
" src/util/strencodings.cpp"\
" src/util/syserror.cpp"\
" src/util/url.cpp"\
Copy link
Member

Choose a reason for hiding this comment

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

Actually, this PR affects more files, e.g., asmap.cpp, readwritefile.cpp etc. Should they be listed here as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

IWYU generates spurious output for some .cpp files, which would require adding headers that I'm not sure are correct, so I've excluded them for now.

These files change infrequently, and not much header shuffling is required.

We don't add everything in src/util/ yet, because IWYU makes some
dubious suggestions, which I'm going to follow up with upstream.
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 07f2c25, I have reviewed the code and it looks OK, I agree it can be merged.

Suggesting to apply clang-format-diff.py as well.

@maflcko maflcko merged commit 1d89fc6 into bitcoin:master Jul 12, 2022
@fanquake fanquake deleted the src_util_iwyu branch July 12, 2022 19:16
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 13, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Sep 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants