IWYU support (part 2): Miscellaneous prep work #27683

jbytheway · 2019-01-17T19:33:08Z

Summary

SUMMARY: None

Purpose of change

To permit IWYU to run cleanly on the codebase requires various tweaks and tidying up. This PR makes the necessary changes. The plan is to follow up with a PR which contains only the changes made by IWYU once this is in. Don't want to put it all together because the header changes make other changes too difficult to find and review.

Describe the solution

Various things, including:

Pragmas to explain things to IWYU when it is unable to understand them.
Manually moving some includes when IWYU moves them wrongly (e.g. it would move them out of a #ifdef).
Adding mapping files to explain to IWYU which files provide what symbols, and which headers are private.
For IWYU to be happy, when a function takes an argument whose type is implicitly convertible from some other type, the header declaring that function must include the header defining that type. See here for more details.
For related reasons, any function return types must be either directly included or forward-declared before running IWYU.
Moved the io::..._archive_tag definitions into a separate header and included it elsewhere instead of forward-declaring.
Removed forward declarations of unit-related types and included the header instead, so that implementation details of units don't leak out.
Un-inlined one move constructor (defining it in the cpp file) to reduce the dependencies in the header which IWYU didn't understand.
Tidied up the SDL headers, so they are almost exclusivley included via the sdl_wrappers header, that deals with the platform-specific issues in one place.
Added some docs about running IWYU.

Describe alternatives you've considered

We could just not do this.

Additional context

To make this work I've also had to patch IWYU. I've made four PRs, of which two have so far been merged. The other two will need to be merged before anyone else can run IWYU without much hassle.

If you're curious as to whether IWYU will actually save us compile time: I'm now finally in a position to answer that, ~~but I haven't done the necessary experiments yet~~ and the results are below. It doesn't make much difference; this is more about maintaining order in the codebase than getting build performance, but at least it means that future efforts towards compile-time optimization ought to be easier.

Add some pragmas to control what IWYU does.

When IWYU auto-moves these to the cpp file they lose the #ifdef

This stuff is only needed on Windows for the subsequent code in the file. On other platformas this header might as well be empty; let's make it so.

These are for gcc in debug mode; different ones would probably be required for other systems.

I see no evidence that they are needed, and if they are they should probably be using sdl_wrappers.h anyway.

The IWYU rule is that when you declare a function taking a class with an implicit conversion (from something), you must provide a definition of that class. e.g. ballistics.h defines projectile_attack, taking an argument of type const dispersion_sources &. dispersion_sources has an implicit conversion from double. Therefore, ballistics.h must include "dispersion.h" (rather than just the forward declaration that was previously present).

All headers used in src/lua/catabindings.cpp need pragmas to prevent IWYU removing them.

IWYU requires forward-declarations (or includes) of function return types in the file defining the function. Moreover, sometimes the lack of same can cause IWYU to break your build, so we must add some up front.

Add pragmas for some header inclusions which IWYU would otherwise remove.

IWYU dislikes using forward declarations as member typedefs, so move these two struct definitions into their own header so it can be included rather than forward-declared.

It needs access to explosion_data destructor, which was not necessarily available after IWYU messed with the headers.

Although technically permissible to IWYU, this ends up being mangled rather, and it seems inappropriate to have the implementation details of the unit tags scattered everywhere.

Fits better with typical include ordering

Move some from header to cpp, and use sdl_wrappers.h where appropriate rather than having more #ifdefs.

jbytheway · 2019-01-17T19:44:59Z

Gorgon isn't happy because I added a blank line to json.h (that was necessary to prevent IWYU thinking that comment was associated with that forward-declaration).

jbytheway · 2019-01-17T21:17:12Z

Have now calculated some build timings. These are times for a from-scratch rebuild with -j4. Debug build with all the features turned on. Three trials for each of three situations.

on master:

real    8m17.082s
user    31m15.490s
sys     1m27.512s

real    8m21.513s
user    31m34.502s
sys     1m27.682s

real    8m14.620s
user    31m11.736s
sys     1m26.803s

on this branch:

real    8m18.163s
user    31m25.603s
sys     1m27.586s

real    8m16.442s
user    31m20.368s
sys     1m27.438s

real    8m16.883s
user    31m19.518s
sys     1m27.470s

after running IWYU on this branch:

real    8m39.643s
user    32m10.881s
sys     1m28.938s

real    8m18.126s
user    31m26.778s
sys     1m26.859s

real    8m16.669s
user    31m21.480s
sys     1m27.112s

So, as expected, not much difference. If anything it might be a little slower.

ZhilkinSerg · 2019-01-18T11:39:33Z

I am not totally sure yet, but I suspect this PR causes linking issues under MSYS (and probably MinGW too):

Possibly related - libSDL2pp/libSDL2pp#66.

ZhilkinSerg · 2019-01-18T15:03:04Z

Android builds were also broken - https://ci.narc.ro/job/Cataclysm-Android/8389/console

06:06:11   /var/lib/jenkins/workspace/Cataclysm-Android/android/app/jni/src/main.cpp:57:24: error: use of undeclared identifier 'read'
06:06:11           if( ( ( rdsz = read( pfd[0], buf, sizeof buf - 1 ) ) > 0 ) ) {
06:06:11                          ^
06:06:11   /var/lib/jenkins/workspace/Cataclysm-Android/android/app/jni/src/main.cpp:77:5: error: use of undeclared identifier 'pipe'
06:06:11       pipe( pfd );
06:06:11       ^
06:06:11   /var/lib/jenkins/workspace/Cataclysm-Android/android/app/jni/src/main.cpp:78:5: error: use of undeclared identifier 'dup2'
06:06:11       dup2( pfd[1], 1 );
06:06:11       ^
06:06:11   /var/lib/jenkins/workspace/Cataclysm-Android/android/app/jni/src/main.cpp:79:5: error: use of undeclared identifier 'dup2'
06:06:11       dup2( pfd[1], 2 );
06:06:11       ^
06:06:11   4 errors generated.
06:06:11   make: *** [/var/lib/jenkins/workspace/Cataclysm-Android/android/app/build/intermediates/ndkBuild/release/obj/local/armeabi-v7a/objs/main/main.o] Error 1

Barhandar · 2019-01-18T15:51:07Z

I am not totally sure yet, but I suspect this PR causes linking issues under MSYS (and probably MinGW too):

Can confirm that I have same issue with compiling latest repo with MSYS.

jbytheway · 2019-01-18T22:32:32Z

I've gone ahead and made the final PR in the set (#27713) because I thought that would probably break both Mingw and Android builds as well, and there's little point fixing them between this one and that one.

jbytheway · 2019-01-21T22:01:32Z

#27713 didn't go through immediately, so I've made PRs (#27755, #27757) that I hope might fix the two issues raised above in the mean time.

jbytheway added 17 commits January 17, 2019 19:13

IWYU pragmas

3e982e6

Add some pragmas to control what IWYU does.

Gap so IWYU doesn't associate commment

18c840c

Move mapsharing includes manually

ddb4979

When IWYU auto-moves these to the cpp file they lose the #ifdef

Move more stuff under #ifdef

65cc5bc

This stuff is only needed on Windows for the subsequent code in the file. On other platformas this header might as well be empty; let's make it so.

Pragma to keep compatibility header

40fcd3f

Add IWYU mapping files

77d12ce

These are for gcc in debug mode; different ones would probably be required for other systems.

Remove SDL includes from player.cpp

68ebf84

I see no evidence that they are needed, and if they are they should probably be using sdl_wrappers.h anyway.

IWYU pragma: keep for lua headers

39089c9

All headers used in src/lua/catabindings.cpp need pragmas to prevent IWYU removing them.

Add forward-declarations of return types

da11168

IWYU requires forward-declarations (or includes) of function return types in the file defining the function. Moreover, sometimes the lack of same can cause IWYU to break your build, so we must add some up front.

Assorted IWYU pragma: keep additions

9f36487

Add pragmas for some header inclusions which IWYU would otherwise remove.

Move io::*_archive_tag to header

5fca293

IWYU dislikes using forward declarations as member typedefs, so move these two struct definitions into their own header so it can be included rather than forward-declared.

Move projectile move constructor to cpp file

c5fe2e0

It needs access to explosion_data destructor, which was not necessarily available after IWYU messed with the headers.

Switch units from forward-declares to includes

2157519

Although technically permissible to IWYU, this ends up being mangled rather, and it seems inappropriate to have the implementation details of the unit tags scattered everywhere.

Move some includes in main.cpp

4a514b3

Fits better with typical include ordering

Tidy up SDL includes

595a7e9

Move some from header to cpp, and use sdl_wrappers.h where appropriate rather than having more #ifdefs.

Provide guidance on how to run IWYU

d27429b

kevingranade merged commit ed196fe into CleverRaven:master Jan 17, 2019

jbytheway deleted the iwyu_prep branch January 18, 2019 08:11

jbytheway mentioned this pull request Jan 18, 2019

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

Closed

This was referenced Jan 21, 2019

Revert main.cpp SDL includes #27755

Merged

Fix Android compile error #27757

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IWYU support (part 2): Miscellaneous prep work #27683

IWYU support (part 2): Miscellaneous prep work #27683

jbytheway commented Jan 17, 2019 •

edited

Loading

jbytheway commented Jan 17, 2019 •

edited

Loading

jbytheway commented Jan 17, 2019

ZhilkinSerg commented Jan 18, 2019

ZhilkinSerg commented Jan 18, 2019

Barhandar commented Jan 18, 2019

jbytheway commented Jan 18, 2019

jbytheway commented Jan 21, 2019

IWYU support (part 2): Miscellaneous prep work #27683

IWYU support (part 2): Miscellaneous prep work #27683

Conversation

jbytheway commented Jan 17, 2019 • edited Loading

Summary

Purpose of change

Describe the solution

Describe alternatives you've considered

Additional context

jbytheway commented Jan 17, 2019 • edited Loading

jbytheway commented Jan 17, 2019

ZhilkinSerg commented Jan 18, 2019

ZhilkinSerg commented Jan 18, 2019

Barhandar commented Jan 18, 2019

jbytheway commented Jan 18, 2019

jbytheway commented Jan 21, 2019

jbytheway commented Jan 17, 2019 •

edited

Loading

jbytheway commented Jan 17, 2019 •

edited

Loading