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

tests: util.h: explicitly include required QString header #14714

Merged
merged 1 commit into from Nov 13, 2018

Conversation

Projects
None yet
7 participants
@1Il1
Copy link
Contributor

commented Nov 13, 2018

Alternative to #14713.

Instead of depending on clang formatter to not reorder includes, another fix is to explicitly include the missing header file.

@ken2812221

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

utACK 27154ce

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

utACK 27154ce
(seems to be the cleaner solution then #14713)

@1Il1

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2018

@jonasschnelli Agreed. I do think the inability to find the main header file is a problem. There is a "regexp for main header file" configuration option as well. Will look into that.

@Empact

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

utACK 27154ce

BTW this is the prefered solution as per the developer notes:

Every .cpp and .h file should #include every header file it directly uses classes, functions or other definitions from, even if those headers are already included indirectly through other headers.

Rationale: Excluding headers because they are already indirectly included results in compilation failures when those indirect dependencies change. Furthermore, it obscures what the real code dependencies are.

https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md

@promag

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

utACK 27154ce.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

utACK 27154ce

Welcome as a contributor @1Il1! :-)

@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

I was afraid for a moment and thought you meant the old src/util.h, which definitely shouldn't include Qt headers

utACK 27154ce ofcourse

@laanwj laanwj added GUI Tests labels Nov 13, 2018

@laanwj laanwj changed the title util.h: explicitly include required QString header tests: util.h: explicitly include required QString header Nov 13, 2018

@laanwj laanwj merged commit 27154ce into bitcoin:master Nov 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Nov 13, 2018

Merge #14714: util.h: explicitly include required QString header
27154ce util.h: explicitly include required QString header (1Il1)

Pull request description:

  Alternative to #14713.

  Instead of depending on clang formatter to not reorder includes, another fix is to explicitly include the missing header file.

Tree-SHA512: f419ef2fd1dfd8da28160a94d187af78463fb398ef6aadd6c68ebf57e6d02380d93f5f370bf2d39e88dcbfeb252c3e5f245c0a157c7d0a64c38fc0f0c7004515

@1Il1 1Il1 deleted the 1Il1:patch-3 branch Nov 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.