Uh oh!
There was an error while loading. Please reload this page.
refactor: spell the public interface <fastfields/...> - #146
Merged
Conversation
437 includes named the public prefix as "fastfields/..." and not one used
<fastfields/...>. That is carryover rather than a decision: the consolidation
script rewrote the *path* in every #include and left the delimiter alone,
because that was the minimal textual change.
include/fastfields/ is the public installed interface -- it is what gets
copied to an install prefix, and what fastfields-dlpack puts on the compiler's
include path via include_dirs. The ordinary convention spells a dependency
found on the include path with <> and a dependency found next to the includer
with "", and this tree was writing both categories the same way. The clearest
evidence that the result reads wrong is downstream, in
fastfields-dlpack/src/ext.cpp, which writes <nanobind/nanobind.h> and
"fastfields/api/distance.h" ten lines apart -- two spellings for the same
category of dependency.
So: <fastfields/...> everywhere for the public prefix, and the ~135 genuinely
relative includes ("../utils.h", "flow/2d.h", "utils.h") stay quoted, because
they are private to their component and correctly spelled already.
WHY THIS CHANGES NOTHING AT COMPILE TIME
"" searches the including file's own directory first and then falls back to
exactly the <> search, so the two differ only when a quoted include is
resolved by that first step. make/common.mk's
INCLUDES += -I$(ROOTDIR)/include
is the entire include configuration -- all four Makefiles compile with
$(INCLUDES) and nothing else, and there is no -iquote anywhere in the tree --
so "" and <> share one search path here.
The script does not ask anyone to take that on trust. Its --check re-derives
it from the sources: it verifies that every quoted include really does resolve
beside its includer (so no quoted include was relying on the include path,
and the rewrite cannot have changed which file any of them names), and the
converse, that no angle-bracket include of ours resolves beside its includer.
Both invariants already held on main before this commit, which is what makes
the conversion mechanical.
NOT TOUCHED
tools/consolidate.sh and tools/dedup-dispatch-helpers.py quote include lines
as literals of the tree at their own parent commit; their documented contract
is to be replayed against that parent, not against today's main. Rewriting
those literals would break the replay, so they are left alone.
REPRODUCIBILITY
tools/normalise-include-delimiters.py is committed, following
tools/rename-macros.py and tools/dedup-dispatch-helpers.py, so a 147-file
rewrite can be replayed instead of read:
git checkout HEAD~1 -- include src tests
python3 tools/normalise-include-delimiters.py
git diff # empty
--check exits non-zero on any violation, so a rebase does not need a hand
audit. The rule is recorded in CLAUDE.md.Uh oh!
There was an error while loading. Please reload this page.
balbasty pushed a commit
that referenced
this pull request
Aug 20, 2026
Resolved by taking main's tree wholesale for include/, src/, tests/ and CLAUDE.md, then re-running the sweep script over the merged base -- the procedure the committed script exists for. #146 (the <fastfields/...> delimiter sweep) and #143 (the per-backend FF_INDEX32 option) both landed underneath this branch and both touch the header preambles this sweep rewrites.
This was referenced Aug 20, 2026
balbasty pushed a commit
that referenced
this pull request
Aug 20, 2026
#146 (angle-bracket includes) and #143 (FF_INDEX32) landed on main while this proposal was being written, which turned the script's central claim from an assertion into something demonstrable. The rebase was done the way the docstring prescribes -- revert, merge main, re-run unedited -- and the same script emitted <fastfields/...> on the new base with no edit and no hand-merging. Both runs are now recorded in Appendix A. Also corrects the vox migration cost: 26 FF_NAMESPACE_BEGIN(<module>) sites across 25 files, not 28 across 26, with the per-module breakdown. The earlier figure came from a scan that counted non-module namespaces (bound, spline, meta, internal, _splinc) alongside the module ones. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z
balbasty pushed a commit
that referenced
this pull request
Aug 20, 2026
… on the new base Regenerated by re-running `tools/move-core-headers.py` unedited on de288a9 (main after #146 and #143), which is the whole point: the earlier prototype commit was reverted rather than hand-merged, and this is what the script produced on its own. 10 headers moved, 4,262 lines 81 files rewritten, now emitting <fastfields/...> instead of "fastfields/..." `--check`: clean, idempotent, no dependency leak Drop this commit and the design in the preceding ones still stands. The one-line tools/test-baseline.sh probe update rides along here because the script does not touch that file: the gate tool probes for impl/kernels/bounds.h to validate tree layout and dies before running a single test if it is absent. tools/consolidate.sh and the six-repo probe in the same script name the same paths and are deliberately left alone -- they describe the frozen pre-consolidation layout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z
balbasty added a commit
that referenced
this pull request
Aug 20, 2026
…ow files Pre-emptive, so that neither sweep has to touch these files and neither conflicts with them: * #145 -- one include-guard convention, no `#pragma once`. reg_flow_slice.h dropped the pragma and its guard is spelled the way that PR derives new ones from the path: FF_SRC_LIB_CUDA_REG_FLOW_SLICE_H. * #146 -- the public interface is spelled <fastfields/...>. Same-directory siblings stay quoted, which is what that PR does to impl/cuda/reg_flow.h's `#include "utils.h"`. Include spelling only; the preprocessed output is unchanged. Front TU still compiles and every slice still preprocesses.
This was referenced Aug 20, 2026
balbasty pushed a commit
that referenced
this pull request
Aug 20, 2026
…d why CI found something no amount of reading would have: the `lint (clang-format, changed lines)` job fails on any file move, because git-clang-format does not honour rename detection. It sees include/fastfields/core/bounds.h as a path absent from the merge base, treats the whole file as changed, and demands all 800 lines conform -- for files that predate the gate and have never been clang-formatted. Measured on the prototype: 4,232 of the 4,236 reformat lines it asks for are that artifact. Only 4 are attributable to anything this change does. Two script fixes, which shrink the diff and are right on their own merits: * Leave any include alone whose existing spelling still resolves from the new location. parallel.h's "parallel_impl.h" needs no edit when both files move together. * Spell everything else absolutely rather than as a bare sibling name. core/ already refers to its own siblings that way (core/dispatch.h -> <fastfields/core/autocast.h>), so the moved files should adopt the convention of the directory they land in rather than import impl/kernels/'s relative style along with them. This also stops the move from fighting #146, which made <fastfields/...> the spelling for the public interface. With both, all ten moved files are byte-identical pure renames (verified by checksum against their originals) and every include edit falls on the 71 files that did not move: 82 files, 140 insertions / 140 deletions, down from 150/150. That does NOT fix the lint -- a zero-content-change rename is flagged just as hard, which is the measurement above. The verified mitigation is to clang-format the ten files in place, in impl/kernels/, as its own PR before the move; formatting a file drops it off the linter's list entirely. That is now written up in the proposal's sequencing section as the item most likely to change how this work is scheduled, with the alternatives (teach the lint to skip R100, or exempt the one PR) and the argument for paying the debt rather than deferring it. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z
This was referenced Aug 20, 2026
balbasty pushed a commit
that referenced
this pull request
Aug 20, 2026
Brings in #143 (per-backend FF_INDEX32), #145 (#pragma once), #146 (<fastfields/...> includes) and #151 (the measured index-axis table). One conflict, in src/lib-cuda/reg_flow.cpp: this branch splits the file and moves the dtype x offset dispatch macros into reg_flow_slice.inl, while #143 rewrote the narrow arm of those same macros from int32_t to off32_t and #146 rewrote the include lines. Resolved by taking this branch's structure with both of main's changes applied in their new home: * all 20 narrow dispatch arms in reg_flow_slice.inl now name off32_t, which is exactly the count main's reg_flow.cpp carries; * the new files already spelled public includes <fastfields/...> and kept the same-directory sibling quoted, so #146 needed nothing; * reg_flow_slice.h switched from an #ifndef guard to #pragma once -- #145 landed the opposite way round from its original proposal, and the new files followed the proposal rather than the merge. tools/normalise-header-guards.py --check, tools/normalise-include-delimiters.py --check and tools/rename-macros.py --check are all clean on the result. Re-verified after the merge: * tools/test-baseline.sh --legs default,lib -> row-for-row identical to tools/test-baseline.expected across all 15 rows. 59,886 checks / 13 suites for the default leg, plus the 2 hub suites, 0 failures. * every one of the 30 slice functions is still defined exactly once across the twelve slice TUs (preprocessor census: 13 occurrences each -- twelve declarations from the shared header plus one definition). * the front TU still instantiates nothing: 205,400 kB, 1.8 s. * the heaviest slice still compiles: 2,037,652 kB, matching the 2,039,660 kB build-cuda reported for it before the rebase (0.1%).
balbasty pushed a commit
that referenced
this pull request
Aug 20, 2026
Four conventions landed after this branch was cut; the new files predate all of them. * #pragma once on line 1, and the whole-file `#ifndef FF_HALF` guard removed (#145). The file had acquired BOTH, which is the one combination the convention rules out -- core/dlpack.h is the sole guard exception and it is vendored, so its upstream macro can collapse with a system DLPack header's. * FF_NAMESPACE_BEGIN/END(FF_NS), not (FF): #91 renamed the root-namespace macro, because a bare two-letter all-caps name in an installed header takes that identifier away from every downstream translation unit. * FF_CUHOSTDEV, not the unprefixed CUHOSTDEV, for the same reason (#91). * <fastfields/...> for the public interface, here and in the three test and probe files that include it (#146). Applied with tools/normalise-header-guards.py, normalise-include-delimiters.py and rename-macros.py; all three now pass --check, as does check-cuda-launches.py. The continuation backslashes in the two clang-format-off macro blocks are re-aligned to the new column so the blocks stay readable. No behaviour change: FF_CUHOSTDEV and CUHOSTDEV expand identically, and the namespace is `ff` either way.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Sweep 2 of two independent mechanical sweeps, deferred until the macro work (#90, #91) merged. The other one — header guards — is #145, deliberately kept separate.
What the tree looked like
Re-counted on
85fdac7(the brief said 406; the merges since added some):"fastfields/…"<fastfields/…>"../utils.h","flow/2d.h","utils.h")<>Carryover, not a decision: the consolidation script rewrote the path in every
#includeand left the delimiter, because that was the minimal textual change.The convention
<fastfields/…>for the public interface,"…"for headers private to the component.include/fastfields/is the public installed interface — what gets copied to an install prefix, and whatfastfields-dlpackputs on the compiler's include path viainclude_dirs. The ordinary convention spells a dependency found on the include path with<>and one found next to the includer with""; this tree was writing both categories the same way.The clearest evidence that the result reads wrong is downstream, in
fastfields-dlpack/src/ext.cpp, which writes<nanobind/nanobind.h>and"fastfields/api/distance.h"ten lines apart — two spellings for the same category of dependency.The ~135 relative includes are left exactly as they are. They are private to their component and correctly quoted already.
Why this changes nothing at compile time — verified, not asserted
""searches the including file's own directory first and then falls back to exactly the<>search, so the two differ only when a quoted include is resolved by that first step.make/common.mkhasand that is the entire include configuration: all four Makefiles compile with
$(INCLUDES)and nothing else, and there is no-iquoteanywhere in the tree (checked acrossmake/, the four Makefiles and both workflows). So""and<>share one search path here.The script does not ask you to take that on trust —
--checkre-derives it from the sources. It verifies:fastfields/…include is still quoted;Invariants 2 and 3 already held on
mainbefore this commit. That is the mechanical safety argument, and it is re-checkable on every rebase rather than being a claim in a PR description.Not touched
tools/consolidate.shandtools/dedup-dispatch-helpers.pycontain quoted-include literals —'#include "fastfields/api/checks.h"'and the consolidation's own rewrite rules. Those describe the tree at their own parent commit, and their documented contract is to be replayed against that parent, not against today'smain. Rewriting the literals would break the replay, so they are deliberately left alone. Flagging it explicitly because agrepfor the old spelling will still hit them.core/dlpack.his vendored and in the script's skip set (it includes nothing of ours in any case).Reproducibility
tools/normalise-include-delimiters.pyis committed, followingtools/rename-macros.pyandtools/dedup-dispatch-helpers.py, so a 147-file / 437-line rewrite can be replayed instead of read:Verified. And:
Not wired into CI, matching the precedent set by
rename-macros.py— and because both of these sweeps would otherwise collide inci.yml. One-line follow-up if you want it as a gate.Validation
tools/test-baseline.sh --tree . --legs all --check tools/test-baseline.expected— row-for-row match, 59,886 checks / 13 suites / 0 failures, unmoved. (Note:--legs default,libcannot be--checked against the recorded file — the script compares whole reports and refuses a leg subset as "NOT COMPARABLE". The full-leg run is what is quoted here.)tools/rename-macros.py --checkclean.make CXX=clang++clean.build-cudaandcompile-probe-cudaare the bar, and no runtime claim is made. This sweep touchessrc/lib-cuda/*.cppandtests/impl-cuda/compile_probe_mesh.cu, so both legs need to be green.Notes
CLAUDE.mdrecords the rule beside theFF_-prefix rule from refactor: prefix every remaining public macro with FF_ #91.CLAUDE.md, where each adds one bullet; whichever lands second may need a one-line rebase there.make/, the per-library Makefiles and the dispatch layer (the per-backend 32-bit-offset work). This PR reads the include flags but changes no build file.fastfields-dlpack/src/ext.cppcan follow suit whenever convenient — nothing here requires it to.Generated by Claude Code