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

[WIP] IWYU support (part 6): Apply IWYU changes (take 2) #27713

Closed
wants to merge 4 commits into from

Conversation

jbytheway
Copy link
Contributor

@jbytheway jbytheway commented Jan 18, 2019

Summary

SUMMARY: Infrastructure "Update header includes to include-what-you-use standards"

Purpose of change

To include what we use :).

Describe the solution

Three more commits with platform-specific fixes, followed by one big commit with the changes made by running IWYU as described in DEVELOPER_TOOLING.md, followed by astyle.

Additional context

Note that although IWYU does sort headers by category, it does not sort alphabetically within categories. I don't have a tool to do that, so things are now non-alphabetical.

Some blank lines have been introduced in places where we probably don't want them. If people feel strongly then I can tidy those up, but would prefer to do so in a separate PR.

This is quite likely to break the Android build.

@jbytheway
Copy link
Contributor Author

Again, Gorgon is complaining because of changes to json.h.

@ZhilkinSerg ZhilkinSerg added [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style labels Jan 18, 2019
@kevingranade
Copy link
Member

Did IWYU get confused by the #ifdef RELEASE in tests/hash_test.cpp?

hash_test.cpp:128:24: error: ‘std::chrono’ has not been declared
     auto start1 = std::chrono::high_resolution_clock::now();
                        ^
hash_test.cpp:138:22: error: ‘std::chrono’ has not been declared
     auto end1 = std::chrono::high_resolution_clock::now();
                      ^
hash_test.cpp:139:17: error: ‘std::chrono’ has not been declared
     return std::chrono::duration_cast<std::chrono::microseconds>( end1 - start1 ).count();
                 ^
hash_test.cpp:139:44: error: ‘std::chrono’ has not been declared
     return std::chrono::duration_cast<std::chrono::microseconds>( end1 - start1 ).count();
                                            ^
Makefile:45: recipe for target 'obj/hash_test.o' failed

@jbytheway jbytheway changed the title IWYU support (part 3): Apply IWYU changes [WIP] IWYU support (part 3): Apply IWYU changes Jan 19, 2019
@jbytheway
Copy link
Contributor Author

Did IWYU get confused by the #ifdef RELEASE in tests/hash_test.cpp?

Yes; I ran in debug mode so it didn't account for uses in RELEASE only.

Will do some more prep and then return to this.

@jbytheway jbytheway changed the title [WIP] IWYU support (part 3): Apply IWYU changes [WIP] IWYU support (part 5): Apply IWYU changes (take 2) Jan 24, 2019
@jbytheway jbytheway changed the title [WIP] IWYU support (part 5): Apply IWYU changes (take 2) [WIP] IWYU support (part 6): Apply IWYU changes (take 2) Jan 24, 2019
@jbytheway jbytheway force-pushed the iwyu_redo_headers branch 2 times, most recently from 7285de8 to 9b229f1 Compare January 25, 2019 22:44
Turns out M_PI isn't standard C, it's POSIX.  In particular, it's not
present on Mingw.  So we need to provide our own fallback definition.
Needed for GetUserDefaultLCID.
These are the changes made by running IWYU as described in
DEVELOPER_TOOLING.md.
@jbytheway
Copy link
Contributor Author

I think this is now ready (finally got a green tick on the Travis CI), but it probably shouldn't get merged until post-0.D because it will lead to all sorts of annoying merge conflicts between master and the release branch. I'll return to it then.

@kevingranade
Copy link
Member

OK this has accumulated a massive list of conflicts, feel free to reopen when you have time and interest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants