IWYU src folder A-F files

[Brambor]

Summary

None

Purpose of change

IWYU changes are good. I stumble upon them randomly

• run IWYU for some files #72906

I decided to run it for whole codebase then. But after a day I only got from A to F, so there it is.

Describe the solution

• Run iwyu, accept changes, fix compilation errors, repeat.
• Sometimes iwyu would say to add "XY.h" and then remove it again, I decided to keep it removed.
• I am using IWYU 0.12 (which is quite behind the newest 0.22).

1. First commit are core changes - run IWYU for files A-F.
2. Second commit contains fixing what first commit broke. IWYU agrees with those changes. I made as few changes as possible.
3. Third commit is like second, but IWYU disagrees. Maybe I could have added IWYU pragma: keep, but I believe it could be solved if IWYU was run over all files.

Describe alternatives you've considered

First run IWYU for all test files. This would be the right thing to do. It would be easy, as tests are independet of each other unlike src/ files. However, I have an old IWYU, which doesn't know IWYU pragma: begin_exports used in (almost?) all tests and I didn't manage to install IWYU 0.20 which adds that, or newer. I then gave up and made this PR.

The benefit of doing tests first instead would be not breaking any test files while changing src includes (since all you use is included not relying on fragile transitive includes). I had to fix some tests affected in the second commit iwyu: snowball changes.

Testing

• Compiled on my machine all that is in src/ and test/. Windows, tiles, in Visual Studio.
• Also compiled through whatever the basic make command is compiling in on Ubuntu 20.04 through WSL, since that's how I get the IWYU output.

Were changes useful?

I don't know how to directly measure compile time apart from letting my PC recompile the whole thing and measure that. I didn't do that.

However, I can count includes in .inc files, so I did that, there are the results:

counting all includes

$ git checkout master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
$ make includes -j 4
(...)
$ find obj/ -name '*.inc' | xargs wc -l
(...)
 218913 total
$ git checkout master
Already on 'master'
Your branch is up to date with 'origin/master'.
$ git checkout iwyu
Switched to branch 'iwyu'
$ make includes -j 4
(...)
$ find obj/ -name '*.inc' | xargs wc -l
(...)
 218879 total

Totally negligable result

Counting only includes from /src

# in /CDDA/
$ git checkout master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
$ make includes -j 4
(...)
$ find obj/ -name '*.inc' | xargs grep "src/" | wc -l
47600
$ git checkout iwyu
Updating files: 100% (193/193), done.
Switched to branch 'iwyu'
$ make includes -j 4
(...)
$ find obj/ -name '*.inc' | xargs grep "src/" | wc -l
47315

0.6% improvement, negligable.

Only count A-F files (but still include all A-Z)

$ git checkout master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
$ make includes -j 4
(...)
$ # delete files outside A-F range
$ find obj/ -name '*.inc' | xargs grep "src/" | wc -l
14730
$ git checkout iwyu
Switched to branch 'iwyu'
$ make includes -j 4
(...)
$ # delete files outside A-F range
$ find obj/ -name '*.inc' | xargs grep "src/" | wc -l
14501

1.5% improvement.

Conclusion

While testing, the transitive includes from G-Z files pollute the relative improvements. The absolute improvement is there, but only the relative improvement can be felt by the programmer, as it says "Compiling will actually take about ~0.6% less time.". That would be 10 seconds out of 1800 seconds.

After these tests, I still think that if IWYU were run for A-Z, transitive includes would be removed much more. I imagine a sigmoid curve. I don't think it would get over 5% reduction in include file count for the whole codebase.

However, for a single change in header time and recompilation, there could be some noticeable compile time reduction, as fewer files need recompilation.

Additional context

IWYU also assumes all .h files have a .cpp file, so I (mostly) didn't touch .h without a .cpp file.

Tools I used (for my future reference)

iwyu find.ahk

#SingleInstance Force

^j::
Send, {Left}{Shift Down}{Ctrl Down}{Left}{Left}{Shift Up}{Ctrl Up}^c!{Tab}
;sleep, 200
;Send, ^w
sleep, 200
Send, ^p^v
sleep, 200
Send, {Enter}
Send, !{Tab}
sleep, 200
Send, {End}
return


; Prerequisite: have (1st) Sublime text window with exactly two tabs: iwyu_parser.py and any(will be closed).
; iwyu_parser.py has empty `includes = ""` and the inside of the string is selected
; some macros so that ^!+2 selects the Sublime Text output
; Have (2nd) text editor with the TODO_iwyu_changes.txt, where human selects the filename and presses ^k.
^k::
; HUMAN select "The full include-list" from after : to before \n---
; copy includes & unselect
Send, ^c{Left}
; goto Python window & tab
Send, !{Tab}
Sleep, 200
Send, ^{Tab}
; run python
Send, ^v^b^z
; tab to TODO
Send, ^{Tab}!{Tab}

sleep, 200
; copy filename
Send, {Left}{Shift Down}{Ctrl Down}{Left}{Left}{Shift Up}{Ctrl Up}^c{End}

; other window
Send, !{Tab}
sleep, 200

; close last file
Send, ^w
sleep, 200

; open new file
Send, ^p^v
sleep, 200
Send, {Enter}

; copy from python build
Send, ^!+2
;already ran for 800 ms
sleep, 100
Send, ^a{Shift Down}{Up}{End}{Shift Up}^c

;Send, !{Tab}
;sleep, 200
return

iwyu_parse.py

includes = """

#include "flexbuffer_cache.h"
#include <flatbuffers/flexbuffers.h>  // for Builder
#include <flatbuffers/idl.h>          // for Parser, IDLOptions
#include <stdio.h>                    // for sscanf, size_t
#include <string.h>                   // for strlen, strncmp
#include <chrono>                     // for milliseconds, duration_cast
#include <cstdlib>                    // for strtoull
#include <fstream>                    // for ofstream, char_traits, basic_is...
#include <limits>                     // for numeric_limits
#include <memory>                     // for shared_ptr, allocator, make_shared
#include <optional>                   // for optional
#include <sstream>                    // for istringstream, basic_istringstream
#include <stdexcept>                  // for runtime_error
#include <system_error>               // for error_code
#include <type_traits>                // for remove_reference<>::type
#include <unordered_map>              // for unordered_map, _Node_iterator
#include <utility>                    // for move, pair
#include <vector>                     // for vector
#include "cata_utility.h"             // for read_maybe_compressed_file, rea...
#include "filesystem.h"               // for remove_file, assure_dir_exist
#include "json.h"                     // for TextJsonValue, TextJsonIn, Text...
#include "mmap_file.h"                // for mmap_file

"""

class IWYU_parser():
	def __init__(self):
		pass

	def parse(self, includes):
		lines = includes.strip().splitlines()
		if not lines:
			return

		last_state = self.parse_state(lines[0])
		for line in lines:
			if "//" in line:
				line = line.split("//")[0].strip()

			# fix namespace
			if line.startswith("namespace"):
				line = self.fix_namespace(line)

			state = self.parse_state(line)
			if state != last_state:
				last_state = self.parse_state(line)
				line = "\n" + line

			print(line)#, state)

	def parse_state(self, line):
		if line.startswith("#include <"):
			return "#include <a>"
		if line.startswith('#include "'):
			return '#include "a"'
		if (line.startswith('class')
			or line.startswith('struct')
			or line.startswith('template')
			or line.startswith('namespace')
		):
			return '-class-'
		print(line)
		raise SyntaxError(repr(line))

	def fix_namespace(self, line):
		assert(line.startswith("namespace"))
		namespace_x, rest = line.split("{")
		namespace = namespace_x.split()[1]

		inside_namespace = rest.split("}")[0].strip()

		return f"namespace {namespace}\n{{\n{inside_namespace}\n}}  // namespace {namespace}"


p = IWYU_parser()
p.parse(includes)

#print(p.fix_namespace("namespace cata { class event; }"))

[github-actions]

Auto-requesting reviews from non-collaborators: @andrei8l

[andrei8l]

Might be worth checking if you can enable clang-tidy's misc-include-cleaner after these changes. It should remove the need for such PRs in the future.

[Brambor]

Might be worth checking if you can enable clang-tidy's misc-include-cleaner after these changes. It should remove the need for such PRs in the future.

I'm 95% sure it would not pass right now, since I didn't finish F-Z. But I would love to do that in the future!

[akrieger]

Would misc-include-cleaner work equivalently to IWYU?

[akrieger]

The analysis is interesting, but I think the ultimate benefit is counting the number of times particular files are included and seeing which decrease the most. For example, it's only a 1.5% reduction in includes for files A-F, but if it's because eg. item.h is included 100 fewer times, that's massively better for changes in item.h.

[Brambor]

Trying to fix clang tidy (include CPP style instead of C style headers).

This might have been improved in the new IWYU include-what-you-use/include-what-you-use#920 which I don't have.

[Brambor]

The two tests failures are clearly unrelated.

[Brambor]

I think it is better to first IWYU src/*.cpp files and test/* files. Only a few, if any, errors would arise. Comparatively little to editing .h files.

When these are done, much less errors should arise IWYU-ing .h files.