[build] Add misc-include-cleaner clang-tidy check for headers - #7054
[build] Add misc-include-cleaner clang-tidy check for headers#7054fhanau wants to merge 1 commit into
Conversation
|
Model not found: cloudflare-ai-gateway/anthropic/claude-opus-4-6. Did you mean: anthropic/claude-opus-4.5, anthropic/claude-opus-4.6, anthropic/claude-opus-4.7? |
|
@fhanau Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
The generated output of |
0eee215 to
c7079e5
Compare
Reduces transitive include bloat by enforcing that no unused headers are being added.
c7079e5 to
1667a06
Compare
| ], | ||
| } | ||
|
|
||
| HEADER_ONLY_CHECKS = [ |
There was a problem hiding this comment.
Note to self: Improve documentation of HEADER_ONLY_CHECKS before landing this
|
|
||
| JsDate::operator kj::Date() const { | ||
| return kj::UNIX_EPOCH + (int64_t(inner->ValueOf()) * kj::MILLISECONDS); | ||
| return kj::UNIX_EPOCH + (static_cast<int64_t>(inner->ValueOf()) * kj::MILLISECONDS); |
There was a problem hiding this comment.
Why am I fixing clang-tidy checks other than misc-include-cleaner in these files? Previously the jsg-core target was not being covered by clang-tidy as some of its headers can't compile on their own due to header interdependencies that I have not been able to resolve. But with the new no-clang-tidy-headers tag, we can apply clang-tidy to the source files only and get proper coverage for them.
Merging this PR will improve performance by 9.18%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | simpleStringBody[Response] |
24.8 µs | 22.7 µs | +9.18% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing felix/080726-include-enforcement (1667a06) with main (84284d8)
Footnotes
-
129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
|
We're still disabling the check in a number of places. Do you plan to cut down on these in a later PR? |
I cleaned up all that I could without significant extra effort:
|
Reduces transitive include bloat by enforcing that no unused headers are being added.
This only covers header files for now and does not include KJ includes, otherwise the PR would need to be much larger and people would get linter errors more frequently.
Deployment plan: This will require some downstream changes to land, a PR for that will follow soon. I plan to only enable this check for workerd for now and see how things go with that, if people find that it is too much of an annoyance to sometimes be told that they have superfluous headers we can disable it again. But I hope that the check makes sense to developers for the most part and that we can benefit from having includes that make sense and slightly faster compile times (fewer includes means less time spent parsing headers).
If feedback is mostly positive a couple weeks in, we may want to extend this to the downstream repo and to capnproto.