Control PR1: the message API leaves the god-header - #1154
Open
aurelienpierre wants to merge 1 commit into
Open
Conversation
control/control.h is included by 125 files. 36 of them use nothing from it but dt_control_log(),
dt_toast_log(), dt_toast_markup_log() or the four busy counters -- twelve are IOP plugins and one
is in src/pixel, two layers below. They pay for it: control.h includes libs/lib.h (a layer-7
header in a layer-3 one, because the progress vtable types its callbacks in dt_lib_module_t) and
hands 25 project headers to every includer.
Those seven declarations move to control/user_message.h, which includes NOTHING -- every
signature is plain C. control.h includes it, so the other 89 includers are untouched.
34 files repointed. Measured fan-in, master -> here:
libs/lib.h 141 -> 107 views/view.h 148 -> 114
control/jobs.h 136 -> 98 control/progress.h 130 -> 92
control/control.h 129 -> 91
Δ layering_violations = 0, as intended: user_message.h sits in control/ at the same layer as the
header it came from, so the wide repoint cannot shift the ratchet in either direction. Moving it
lower is a later step and a separate argument.
THREE of the 37 candidates were NOT repointed, which is the part worth reading. The classifier
asked "does this file use any symbol DECLARED in control.h", and that is the wrong question --
CLAUDE.md says so directly: the "files using nothing from it" bucket means the file reaches those
symbols through the include, so it needs one added EXPLICITLY, not removed. The build found all
three:
- common/pwstorage/pwstorage.c reached IS_NULL_PTR and _() through control.h. Both come from
layers at or below it, so it keeps the repoint and gains system/macros.h and <glib/gi18n.h>
explicitly. That is the honest outcome: the dependency was always there, now it is stated.
- gui/presets.c (dt_lib_presets_remove) and iop/lut3d.c (dt_lib_module_t, mouse_scroll) reached
libs/lib.h through control.h. Adding it explicitly would be gui->libs and iop->libs, both
UPWARD: the ratchet would rise. They are not message-only files, so they keep control.h until
the libs/lib.h edge itself goes.
Release, Debug (-Werror) and nofeatures all build; ctest 8/8; module boundaries hold. The
nofeatures build is not ceremony here -- it is where a removed supply line surfaces, per the
colorprofiles split.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR1 of
doc/control-split.md(#1152). Depends on nothing; #1153 fixed the tool that made this measurable.control/control.his included by 125 files. 36 of them use nothing from it butdt_control_log(),dt_toast_log(),dt_toast_markup_log()or the four busy counters — twelve are IOP plugins, and one is insrc/pixel, two layers below. They pay heavily for it:control.hincludeslibs/lib.h(a layer-7 header inside a layer-3 one, because the progress vtable types its callbacks indt_lib_module_t) and so hands 25 project headers to every includer.Those seven declarations move to
control/user_message.h, which includes nothing at all — every signature is plain C.control.hincludes it, so the other 89 includers are untouched.Measured fan-in, master → here
libs/lib.hviews/view.hcontrol/jobs.hcontrol/progress.hcontrol/control.hlayering_violationsΔ = 0, as the plan intends:user_message.hsits incontrol/at the same layer as the header it left, so a 34-file repoint cannot move the ratchet in either direction. Moving it lower is a later step and a separate argument.The part worth reading: three files were NOT repointed
My classifier asked "does this file use any symbol declared in
control.h", and that is the wrong question. CLAUDE.md says so directly — the "files using nothing from it" bucket means the file reaches those symbols through the include, so it needs one added explicitly, not removed. The build caught all three, and they split two ways:common/pwstorage/pwstorage.creachedIS_NULL_PTRand_()throughcontrol.h. Both live at or below its own layer, so it keeps the repoint and gainssystem/macros.hand<glib/gi18n.h>explicitly. That's the honest outcome — the dependency was always there, it's now stated.gui/presets.c(dt_lib_presets_remove) andiop/lut3d.c(dt_lib_module_t,mouse_scroll) reachedlibs/lib.hthroughcontrol.h. Adding it explicitly would begui→libsandiop→libs, both upward — the ratchet would rise. They aren't message-only files, so they keepcontrol.huntil thelibs/lib.hedge itself goes (PR5).Verification
Release, Debug (
-Werror) and nofeatures all build;ctest8/8;cycles 0,layering_violations 184(baseline); module boundaries hold.build-nofeaturesisn't ceremony here — it's exactly where a removed supply line surfaces, per thecolorprofiles/colorspaces.hsplit. It was in fact the first config to go green while the other two still failed, which is the reverse of the usual order and a reminder that one green config proves nothing.🤖 Generated with Claude Code