-
-
Notifications
You must be signed in to change notification settings - Fork 722
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
refactor: only include what you use #3398
Conversation
e2479fd
to
6b95c6f
Compare
We could absolutely eliminate almost all the includes in each cpp file by just including a single pch.hpp file that includes everything else, but that would complicate the dependency. It is chaotic because of how the individual units are linked together. e.g.
|
If lines of code is important as a metric, I think another approach is to include all the use std and dependency library headers into a single common header and include it in each required file instead. But separating the headers owned by Hyprland could still be valuable for development, as it means fewer rebuilds and fewer complaints from the language server. |
well, including all the stl shit into a common.hpp file would be bad the other way - increase compile times massively. Up till now I have been trying to strike a balance between the two. |
There is another more conservative branch which only touches headers from /src, the diff should be half this size. |
where |
wait, since this is just a util, can't we make it run before build, write the new source files to ./build and compile that? I believe CMake would not have issues with that |
You mean something like LLVM's clang-include-cleaner/fixer or Google's iwyu? |
hm, never mind then, sucks. |
bb93090
to
3a189c2
Compare
7110a04
to
25aec3a
Compare
fb471b8
to
1237732
Compare
Just personal opinion, I believe the best practice is still to include only and directly what it uses. Maybe step by step, but new include should follow the rule. Share common includes in several headers maybe good enough at the beginning, but it's likely to cause more trouble as time goes by. Tens of includes on top of every file may be chaotic to some degree, but it's still necessary code to keep things explicit and straight forward. Just like the code for other functions or classes, it's there for a reason, and can be ignored too when you're focused on something else. |
Currently, header usage in Hyprland is rather chaotic, some headers have circular dependencies, some headers or source includes have more than they actually need.
This can cause unexpected compiler/linker errors and increase maintenance costs. It also slows down PCH disabled and incremental builds. Since unnecessary files are included, the build system will recompile where it's not necessary.
Another problem is with the development tools, especially the language servers such as clangd, where it reports mysterious errors in headers included in headers included in headers included in headers included in headers...
Thus this PR propose that each source file should contain only and directly what it uses.
Advantages
Reduces incremental build time
For example, suppose we do an initial build with PCH disabled (required for compile caching), then modify KeybindManager.hpp and corresponding .cpp to introduce a new dispatcher, and finally incrementally rebuild the modified files.
Speed up: avg 350%
Clearly defined dependencies
Continuing with the example above, in the main branch it is unclear how many files depend on KeybindManager, even though there are only three instances of `#include "KeybindManager.hpp", a change in the header results in in 49 modified files.
In this PR, the dependant of KeybindManager can be easly found using a simple grep 1,
$ grep KeybindManager.hpp -lR src | grep -v pch src/config/ConfigManager.cpp src/debug/HyprCtl.cpp src/events/Windows.cpp src/helpers/Monitor.cpp src/layout/DwindleLayout.cpp src/layout/MasterLayout.cpp src/layout/IHyprLayout.cpp src/managers/input/InputManager.cpp src/managers/KeybindManager.cpp src/plugins/PluginAPI.cpp src/Window.cpp src/Compositor.cpp
Footnotes
Some are not hard dependencies and further improvements can be made. ↩