Uh oh!
There was an error while loading. Please reload this page.
refactor: #pragma once in every header - #145
Merged
Merged
Conversation
The tree carried four conventions across 117 headers -- 94 with an #ifndef guard only, 12 with `#pragma once` only, 5 with both, and 6 umbrella headers in impl/kernels/ with neither. The split is by provenance: each of the six absorbed repositories had drifted its own way, and consolidation put them side by side without picking one. The convention is now: every header is bracketed by an #ifndef/#define/#endif guard, opened before any other directive and closed as the last directive in the file, and no header uses `#pragma once`. WHY GUARDS Both mechanisms work, and the historical objection to `#pragma once` here -- that every cross-repo dependency was a symlink, so "the same file" had several identities -- really is gone: main has no .gitmodules, no gitlinks and no symlinks. Portability does not decide it either; clang, g++, nvcc and MSVC all support the pragma. What decides it is what include/fastfields/ is: the public *installed* interface, copied to an install prefix and consumed from there by fastfields-dlpack. `#pragma once` keys on file identity, so the build-tree copy reached through -I include and the installed copy reached through the prefix are two files: in one translation unit that sees both, the pragma includes both and the second redefines everything the first defined. A macro guard keys on a name, so the two copies collapse into one inclusion. That is not an invented worry. It is exactly why core/dlpack.h must keep its upstream DLPACK_DLPACK_H_ guard -- so our vendored copy interoperates with a system DLPack header. The property the vendored file needs is the property an installed header needs, and it would be odd for the public surface to hold itself to a weaker rule than the one file everybody already agrees must be guarded. Guards are also the only choice under which dlpack.h conforms as it stands rather than being a carve-out: the convention costs this tree zero exemptions. "Both" was in use in five headers and is defensible, but it buys nothing over the guard alone -- in every case where the two disagree it is the guard that does the work -- at the cost of a second thing to keep in sync in 117 files. WHAT CHANGED * 12 pragma-only headers (all impl/cuda/) and the 6 unguarded umbrella headers gain a guard; * 5 "both" headers lose the redundant pragma, keeping their guard name; * impl/kernels/pushpull/nd.h closed with `#endif FF_PUSHPULL_ND` -- extra tokens after #endif, which is ill-formed and diagnosed under -pedantic; * impl/cpu/distance_mesh.h closed with a comment naming a different macro (FF_DISTANCE_MESH) than its guard (FF_CPU_DISTANCE_MESH). The six unguarded headers were harmless in that they contain nothing but #include lines, so double inclusion was a no-op -- but one added definition away from not being. GUARD NAMES A guard this pass *adds* is derived from the file's path under include/fastfields/: impl/cuda/utils.h -> FF_IMPL_CUDA_UTILS_H. Derivation makes the name unique by construction and gives new headers a rule to follow rather than a precedent to guess at. The 99 existing guards keep their names. They are already unique and already FF_-prefixed, so renaming them buys a reader nothing, invalidates the literal guard text quoted in the frozen tools/consolidate.sh, and multiplies the conflict surface against the long-running teeny branch. What had actually drifted is the *shape* of the guard, and that is what is normalised; --check enforces the properties that matter (present, whole-file, FF_-prefixed, unique) rather than a spelling. REPRODUCIBILITY tools/normalise-header-guards.py is committed, as tools/rename-macros.py and tools/dedup-dispatch-helpers.py were, so the rewrite can be replayed instead of read: git checkout HEAD~1 -- include && python3 tools/normalise-header-guards.py git diff # empty `--check` verifies the convention and exits non-zero on any violation, so a rebase does not need a 117-file hand audit. It is idempotent and it leaves core/dlpack.h untouched. The rule is recorded in CLAUDE.md beside the FF_-prefix rule.
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.
The tree carried four conventions across 117 headers -- 94 with an #ifndef guard only, 12 with `#pragma once` only, 5 with both, and 6 umbrella headers in impl/kernels/ with neither. The split was by provenance: each of the six absorbed repositories had drifted its own way, and consolidation put them side by side without picking one. The convention is now `#pragma once`, on line 1 of every header, with no `#ifndef` include guards. Line 1 with no exception for a licence or provenance comment: "is line 1 `#pragma once`" needs no judgement to apply and none to check, and it is what the twelve headers already using the pragma did. Attribution comments keep their text and sit one line lower. `#pragma once` is not ISO C++, but clang, g++, nvcc and MSVC all support it, so it costs nothing against the stated Windows goal. The historical hazard -- that it keys on file identity, which was ambiguous when every cross-repo dependency was a symlink -- is gone: main has no .gitmodules, no gitlinks and no symlinks. THE ONE EXCEPTION: core/dlpack.h Vendored verbatim, and it keeps its upstream `DLPACK_DLPACK_H_` guard. That macro is what lets our copy and a *system* DLPack header carrying the same guard collapse into a single inclusion -- something `#pragma once` cannot do, because those are two distinct files. The script never touches it; --check treats it as exempt but still verifies it kept the guard, and fails loudly if the path disappears rather than silently dropping the exemption. Seven other headers carry third-party provenance: impl/kernels/atomic.h "CUDA portion copied from PyTorch/ATen" impl/kernels/parallel.h "adapted from PyTorch/ATen ParallelNative" impl/kernels/parallel_impl.h ditto impl/kernels/threadpool.h YasserAsmi/wstpool (MIT) impl/kernels/threadpool.inl "some of this is copied from pytorch/aten" impl/cuda/utils.h two helpers "(Copied from PyTorch)" impl/kernels/distance/mesh.h .../TriangleMeshDistance All seven get the pragma, because all seven are adaptations rather than drop-in vendored copies. The test that settles it is not how much text came from upstream but whether the file carries an upstream *guard macro* to interoperate with, and none does: each is guarded by a name this project invented (FF_ATOMIC, FF_PARALLEL_H, FF_THREADPOOL_H, ...) or -- impl/cuda/ utils.h -- by nothing at all, having already used the pragma. They are also re-namespaced into ff:: via FF_NAMESPACE_BEGIN, use this project's FF_CUHOST /FF_CUDEV qualifiers and include project headers, so no upstream copy could substitute for them and none is on any include path. Copyright notices are untouched either way; this is include mechanics, not attribution. REMOVING A GUARD IS NOT AUTOMATICALLY INERT A guard macro can be *tested* from outside the header that defines it, and then deleting its `#define` silently changes what compiles -- the one way a sweep like this breaks something with no diagnostic. So the script greps the whole repository for every guard macro before removing any, and refuses to run if one is mentioned in a compiled source outside its own header. --check runs the same audit, so a future header that starts testing a guard name turns it red. The audit is clean on this base: all 99 guard macros are referenced exactly once, by their own `#ifndef`, with no `#ifdef` on a guard name anywhere in include/, src/ or tests/. The only mentions elsewhere are inert text -- tools/consolidate.sh quoting header text it generated, one prose line in MIGRATION-PROVENANCE.md, and the script's own docstring -- and those are reported rather than blocking. WHAT IS NOT A HEADER GUARD, AND SURVIVES Only a guard bracketing the whole file is removed. Partial-file `#ifndef` blocks stay, and two families matter: * FF_LIB_BOUND_SPLINE_T -- eight api/*.h headers wrap the shared bound_t/ spline_t declarations in it so they can be co-included. That is precisely the job `#pragma once` cannot do (one macro, eight files), and it is depended on downstream: fastfields-dlpack/src/ext.cpp includes all eight and says so in a comment. * FF_POSDEF_MAX_NBATCH, FF_PP_MAX_NBATCH, FF_RESIZE_MAX_NBATCH, FF_RESTRICT_MAX_NBATCH, FF_SPLINC_MAX_NBATCH, FF_AUTOCAST_PINNED_HOST -- `#ifndef X / #define X <value>` overridable build knobs. The guard finder only accepts an `#ifndef` that is the file's first directive, whose `#define` has an empty replacement list, and whose `#endif` is the file's last directive, so none of those can be mistaken for a guard. ALSO FIXED, BECAUSE THE SWEEP DELETED THE LINES * impl/kernels/pushpull/nd.h closed with `#endif FF_PUSHPULL_ND` -- extra tokens after #endif, which is ill-formed and diagnosed under -pedantic; * impl/cpu/distance_mesh.h closed with a comment naming a macro (FF_DISTANCE_MESH) that was not its guard (FF_CPU_DISTANCE_MESH); * impl/kernels/distance.h had no trailing newline. REPRODUCIBILITY tools/normalise-header-guards.py is committed, as tools/rename-macros.py and tools/normalise-include-delimiters.py were, so the rewrite is replayed rather than read: git checkout HEAD~1 -- include && python3 tools/normalise-header-guards.py git diff # empty --check verifies the convention and exits non-zero on any violation, so a rebase does not need a 117-file hand audit. It is idempotent, and everything it checks is per-file content rather than path, so it keeps working if headers move between directories. The rule is recorded in CLAUDE.md beside the FF_-prefix rule, dlpack exception and reason included.
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
commented
Aug 20, 2026
CollaboratorAuthor
ok. clang-format should be run for information alone, not make the CI fail (we know we are not linted) |
Uh oh!
There was an error while loading. Please reload this page.
balbasty pushed a commit
that referenced
this pull request
Aug 20, 2026
Modify/delete conflict on all 11 impl/kernels/vector/ headers: #145 added #pragma once to them while this branch deletes them. Resolved in favour of the deletion -- they are unreferenced by include/ and src/, and their only consumer (a compiled-but-never-run scratch target) goes with them. Verified after resolution: no surviving reference to kernels/vector anywhere in include/, src/, tests/, make/ or the Makefile.
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%).
This was referenced Aug 20, 2026
Merged
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 was referenced Aug 20, 2026
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 1 of the two mechanical sweeps deferred until the macro work (#90, #91) merged. Sweep 2 is #146, now merged.
Rebased onto
de288a9— #146 (<fastfields/…>, same header preambles) and #143 (FF_INDEX32) both landed underneath this branch. Resolved by takingmain's tree wholesale forinclude/,src/,tests/andCLAUDE.md, then re-running the sweep script over the merged base. No hand-editing of headers; a clean--checkis the evidence.What the tree looked like
Re-counted on the merged base (the brief's pre-merge figures were 88/8/7/6 across 109 headers):
#ifndefguard only#pragma onceonlySplit by provenance, not by any decision — the 12 pragma-only headers are all
impl/cuda/, i.e. all offastfields-cuda-impl; the 6 unguarded ones are the pure umbrella headers inimpl/kernels/.The convention
#pragma onceon line 1 of every header, no#ifndefinclude guards.Line 1 with no exception for a licence or provenance comment: "is line 1
#pragma once" needs no judgement to apply and none to check, and it is what the twelve headers already using the pragma did. Attribution comments keep their text and sit one line lower.#pragma onceis not ISO C++, but clang, g++, nvcc and MSVC all support it, so it costs nothing against the stated Windows goal. The historical hazard — that it keys on file identity, which was ambiguous when every cross-repo dependency was a symlink — is gone:mainhas no.gitmodules, no gitlinks and no symlinks.104 of the 116 non-vendored headers changed. The 12 that did not are the
impl/cuda/headers that already had the pragma on line 1.The one exception:
core/dlpack.hUntouched, and confirmed after the rebase: still
#ifndef DLPACK_DLPACK_H_/#define DLPACK_DLPACK_H_, zero occurrences of#pragma once, byte-identical tomain.It is vendored verbatim, and that macro is what lets our copy and a system DLPack header carrying the same guard collapse into one inclusion — which
#pragma oncecannot do, because those are two distinct files. The script never touches it;--checktreats it as exempt but still verifies it kept the guard, and fails loudly if the path disappears rather than silently dropping the exemption when files move.Is anything else vendored?
Seven other headers carry third-party provenance. I checked each rather than assuming, and all seven get the pragma:
impl/kernels/atomic.himpl/kernels/parallel.himpl/kernels/parallel_impl.himpl/kernels/threadpool.himpl/kernels/threadpool.inlimpl/cuda/utils.himpl/kernels/distance/mesh.hThe test that settles it is not how much text came from upstream but whether the file carries an upstream guard macro to interoperate with — that is the entire reason
dlpack.hkeeps its guard. None of the seven does: each is guarded by a name this project invented (FF_ATOMIC,FF_PARALLEL_H,FF_THREADPOOL_H, …), or —impl/cuda/utils.h— by nothing at all, since it already used the pragma. They are also re-namespaced intoff::viaFF_NAMESPACE_BEGIN, use this project'sFF_CUHOST/FF_CUDEVqualifiers and include project headers, so no upstream copy could substitute for them and none is on any include path. There is nothing for a guard to interoperate with. Copyright notices are untouched either way — this is include mechanics, not attribution.dlpack.hremains the only verbatim third-party file in the tree.Removing a guard is not automatically inert — audited, not assumed
A guard macro can be tested from outside the header that defines it (
#ifdef FF_SPLINE), and then deleting its#definesilently changes what compiles. That is the one way a sweep like this breaks something with no diagnostic, so it is a step in the script, not a spot check: it greps the whole repository for every guard macro before removing any, and refuses to run if one is mentioned in a compiled source outside its own header.--checkruns the same audit, so a future header that starts testing a guard name turns it red.Re-run against the merged tree (post-#143, post-#146): all 99 guard macros are referenced exactly once, by their own
#ifndef. No#ifdef/#if definedon a guard name anywhere ininclude/,src/ortests/.#143's new build-time macroFF_INDEX32is not a header guard and is untouched.The only mentions anywhere else are inert text —
tools/consolidate.shquoting header text it generated, one prose line inMIGRATION-PROVENANCE.md, and the script's own docstring. Those are reported, not blocking, since they cannot affect what compiles.What is not a header guard, and survives
Only a guard bracketing the whole file is removed. Partial-file
#ifndefblocks stay, and two families matter:FF_LIB_BOUND_SPLINE_T— eightapi/*.hheaders wrap the sharedbound_t/spline_tdeclarations in it so they can be co-included. This is exactly the job#pragma oncecannot do (one macro, eight files), and it is depended on downstream:fastfields-dlpack/src/ext.cppincludes all eight and has a comment saying so. Verified untouched.FF_POSDEF_MAX_NBATCH,FF_PP_MAX_NBATCH,FF_RESIZE_MAX_NBATCH,FF_RESTRICT_MAX_NBATCH,FF_SPLINC_MAX_NBATCH,FF_AUTOCAST_PINNED_HOST—#ifndef X / #define X <value>overridable build knobs.The guard finder only accepts an
#ifndefthat is the file's first directive, whose#definehas an empty replacement list, and whose#endifis the file's last directive — so none of the above can be mistaken for a guard. It scans directives with comments blanked and tracks conditional nesting, rather than pattern-matching line 1.Also fixed, because the sweep deleted the lines
impl/kernels/pushpull/nd.hclosed with#endif FF_PUSHPULL_ND— extra tokens after#endif, ill-formed and diagnosed under-pedantic;impl/cpu/distance_mesh.hclosed with a comment namingFF_DISTANCE_MESHwhile its guard wasFF_CPU_DISTANCE_MESH;impl/kernels/distance.hhad no trailing newline.The 6 unguarded headers were harmless today — nothing but
#includelines, so double inclusion was a no-op — but one added definition away from not being.Reproducibility
tools/normalise-header-guards.pyis committed, astools/rename-macros.pyandtools/normalise-include-delimiters.pywere:Verified, including after the rebase — which is how the merge was resolved.
Everything
--checkverifies is per-file content rather than path (the sole exception being the one-entry vendored list, which fails loudly rather than silently if that file moves), so it keeps working if headers move between directories.Not wired into CI, matching the precedent set by
rename-macros.py. 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. (--legs default,libcannot be--checked against the recorded file: the script compares whole reports and refuses a leg subset as "NOT COMPARABLE".)tools/rename-macros.py --checkclean;tools/normalise-include-delimiters.py --checkclean (refactor: spell the public interface <fastfields/...> #146's convention still holds after the rebase).make CXX=clang++clean.build-cuda— now both theindex32andindex64legs from build: make the 32-bit index axis a per-backend option (FF_INDEX32), default unchanged #143, i.e. every header compiled in two CUDA configurations — andcompile-probe-cudaare the bar. No runtime claim is made.Notes
CLAUDE.mdrecords the rule beside theFF_-prefix rule, including thedlpack.hexception and its reason, the adaptation-vs-vendored test, and the partial-#ifndefcarve-out, so nobody "fixes" any of them later.include/,CLAUDE.mdand one new file intools/— no build files, so it stays clear of build: make the 32-bit index axis a per-backend option (FF_INDEX32), default unchanged #143's territory.