Skip to content

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Sep 11, 2021

This is a header cleanup after #20788.

@hebasto
Copy link
Member Author

hebasto commented Sep 11, 2021

cc @vasild

@vasild
Copy link
Contributor

vasild commented Sep 13, 2021

Just out of curiosity - what drove you to do this change? Did you use some tool which suggested these changes?

It is somewhat difficult to asses whether removing #include <asysheader> from foo.cpp is justified - how to check that foo.cpp is not using any of the functions from that header?

I see why <codecvt> was moved from netbase.cpp to util/sock.cpp - the windows-only variant of NetworkErrorString() is using std::codecvt_utf8_utf16 and that function was moved from netbase.cpp to util/sock.cpp (but the header was not moved).

Why remove <cwchar> from util/sock.cpp? Looks like in order to confirm this change is legit one has to verify that none of the symbols from https://en.cppreference.com/w/cpp/header/cwchar are used in util/sock.cpp. Is there an easy way to do that?

Same for making <locale> windows-only. How to check that non-windows code is not using any of its symbols?

@hebasto
Copy link
Member Author

hebasto commented Sep 13, 2021

@vasild

Just out of curiosity - what drove you to do this change?

Reading dec9b5e commit while working on #20744.

Did you use some tool which suggested these changes?

No. The added code is short enough to manually verify need of #include <cwchar> and #include <locale>.

Why remove <cwchar> from util/sock.cpp?

Why was it added in dec9b5e? I see no reason for that.

Same for making <locale> windows-only. How to check that non-windows code is not using any of its symbols?

Same. I see no reason to #include <locale> for non-Windows platforms in dec9b5e.

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK 3174425

Alright, thanks for the clarifications, @hebasto. I don't remember why I added <cwchar>, surely it was not without a reason, maybe some windows CI failed on me. Anyway, now it looks like it is not needed and CI is green.

Is it time to integrate https://include-what-you-use.org/ into the project?

@hebasto
Copy link
Member Author

hebasto commented Sep 14, 2021

Is it time to integrate https://include-what-you-use.org/ into the project?

#15442

@DrahtBot
Copy link
Contributor

Guix builds

File commit 2161a05
(master)
commit 629b86f
(master and this pull)
SHA256SUMS.part a9a375ae5d3d7897... 63a66ef0d48aa319...
*-aarch64-linux-gnu-debug.tar.gz 0ffaab33cf740e46... a0d36982affe1785...
*-aarch64-linux-gnu.tar.gz b41ca36d3fed9c2f... b28537bb715e434c...
*-arm-linux-gnueabihf-debug.tar.gz 1ac79064c91854ac... b282ec23a240851e...
*-arm-linux-gnueabihf.tar.gz 5399e4a5ea61ccd1... a2abefc08cf75454...
*-osx-unsigned.dmg 1ca70ce0b4b48e27... 0d782c6ef8ad5027...
*-osx-unsigned.tar.gz e7d44f030cbdbd03... 61b51e3f1ba798e1...
*-osx64.tar.gz de52a250ed0768cd... ee042cc7919f0bdc...
*-powerpc64-linux-gnu-debug.tar.gz 0966df174570e7b6... f01146ec5f4f6b31...
*-powerpc64-linux-gnu.tar.gz 465af3689d999d1f... 682c6f17c21d1926...
*-powerpc64le-linux-gnu-debug.tar.gz 639887f5ab3a24eb... ed84b47d2320091e...
*-powerpc64le-linux-gnu.tar.gz ba92b866267f368e... 95b7528cb2539f77...
*-riscv64-linux-gnu-debug.tar.gz 5e51f8e7ca52cf5c... 7bb6710a7fe75aba...
*-riscv64-linux-gnu.tar.gz 3d362954996f46b2... 9285f0a685396098...
*-win-unsigned.tar.gz 2fe1344f94a4ec4c... fde7b27d2a848815...
*-win64-debug.zip e0054a82e946aa6c... 55e77671a14ccebc...
*-win64-setup-unsigned.exe e327f17bc9c9af84... beeadf2acf423425...
*-win64.zip f649f697b0dbad16... 89ee5423b1664e47...
*-x86_64-linux-gnu-debug.tar.gz daf5d05b8b85d09a... 150c6c36e3100936...
*-x86_64-linux-gnu.tar.gz 9930b7ae5591f1d8... af015fbb7894a20b...
*.tar.gz 8815d09678e076d6... 05935b765c18b5f6...
guix_build.log 646ed421098563a1... 3be591ba7433c27f...
guix_build.log.diff 6e22cfdf51222c3b...

maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Sep 16, 2021
3174425 Cleanup headers after #20788 (Hennadii Stepanov)

Pull request description:

  This is a header cleanup after #20788.

ACKs for top commit:
  vasild:
    ACK 3174425

Tree-SHA512: 1c21b1ba43841880625289174f10e5b333f6eb857f448e1e4114b1ecdf32a6044ec91c5987c1d66806c1d408a4e3d46569eb41d69a0acb8296601d7c203d9f1d
@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@fanquake fanquake closed this Sep 16, 2021
@hebasto hebasto deleted the 210911-headers branch September 16, 2021 08:43
@maflcko
Copy link
Member

maflcko commented Sep 16, 2021

This was merged, but not marked as merged by GitHub.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 19, 2021
3174425 Cleanup headers after bitcoin#20788 (Hennadii Stepanov)

Pull request description:

  This is a header cleanup after bitcoin#20788.

ACKs for top commit:
  vasild:
    ACK 3174425

Tree-SHA512: 1c21b1ba43841880625289174f10e5b333f6eb857f448e1e4114b1ecdf32a6044ec91c5987c1d66806c1d408a4e3d46569eb41d69a0acb8296601d7c203d9f1d
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
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.

5 participants