Skip to content

refactor: prefix every remaining public macro with FF_ - #91

Merged
balbasty merged 3 commits into
mainfrom
refactor/prefix-public-macros
Aug 20, 2026
Merged

refactor: prefix every remaining public macro with FF_#91
balbasty merged 3 commits into
mainfrom
refactor/prefix-public-macros

Conversation

@balbasty

Copy link
Copy Markdown
Collaborator

PR 2 of 2. Stacked on #90 — review and merge that first. (Base is
refactor/dedup-shared-macros; it will retarget to main automatically when
#90 lands.)

#90 de-duplicated the copy-pasted dispatch helpers and prefixed the ones it
moved. This finishes the job on the rest of the installed surface.

The rule

Every macro that survives preprocessing of a header under include/ — every
#define not #undef'd before the end of the header that defined it — must
be spelled FF_*.

Installing a header means every translation unit downstream inherits its
macros; an unprefixed one is a name this project has silently taken from
everybody who includes us. 184 macros here already followed the convention
— it existed, it just was not applied. This is the remaining 31, minus the
exemptions.

Macros defined and consumed inside a single .cpp are not covered — they
never leave the translation unit. That is why the dispatch pyramid's local
NDIM_SWITCH / BOUND_SWITCH / MV_ARGS / KN_ARGS / DG_ARGS / *_DT /
BND1..3 are untouched: they are TU-private, and their design is a separate
change in other hands.

What changed

fromtowhere
FFFF_NScore/defines.h — see below
CUGLOBCUHOSTCUDEVCUHOSTDEVFF_CUGLOBcore/cuda_switch.h
DISPATCH_PPFF_DISPATCH_PPapi/{cpu,cuda}/pushpull_dispatch.h
PP_BOUNDPP_DTYPEPP_ORDERFF_PP_*same — renamed, not redesigned
ATOMIC_INTEGER_IMPL, GPU_ATOMIC_INTEGERFF_*impl/kernels/atomic.h
INTERPOL_UTILSFF_INTERPOL_UTILSimpl/kernels/spline.h
DIST_USE_LOOPFF_DIST_USE_LOOPimpl/kernels/distance/mesh.h
JFH_OnePlusTinyFF_ONE_PLUS_TINYand de-duplicated — it was #defined identically in three sibling posdef/ headers; only posdef/utils.h keeps it, the other two already include it
uchar_tdeletedexpanded to unsigned char at its six uses

uchar_t is deleted rather than renamed. A lowercase macro impersonating a
typedef is worse than a shouty one: a downstream
typedef unsigned char uchar_t; does not merely collide, it fails to compile
with a diagnostic pointing at the wrong file.

DISPATCH_PPFF_DISPATCH_PP, not FF_PP_DISPATCH, because
impl/cuda/pushpull.h already uses that second spelling. It #undefs it so
there is no real collision — but reusing the name would be needlessly confusing
to read.

FF itself — the verdict, and the reasoning

Renamed to FF_NS. 253 use sites across 105 files, 228 of which are
FF_NAMESPACE_BEGIN(FF) / FF_NAMESPACE_END(FF).

#undef FF at the end of core/defines.h was considered and is not an
alternative — it is actively wrong.
FF is consumed by ~105 other files
after they include defines.h. Undefining it at the bottom of the defining
header would make every FF_NAMESPACE_BEGIN(FF) expand to a namespace
literally named FF — not a compile error, just silently the wrong
namespace in every file. That is strictly worse than the status quo. The
#undef idiom only works for a macro used within the header that defines it,
which is not this one.

Keeping it with a documented rationale was the real alternative. Rejected,
on three grounds:

  1. It is the worst offender on the list — two letters, all caps, installed
    header — and FF is an entirely plausible downstream identifier (an enum
    member, a constant, a template parameter). "Anything on the installed public
    surface must be prefixed" does not get an exception for the macro that most
    needs it.
  2. Unlike most wide renames, this one is compiler-verified: a missed site
    is an undeclared identifier, never a silent behaviour change. The blast
    radius is large but the risk is not.
  3. The churn argument is real but weak here. It is one deterministic script,
    and if the base moves the fix is to re-run it rather than resolve conflicts.

The indirection is kept deliberately — headers must not hard-code ff::
so the macro is renamed, not removed. core/defines.h now carries the
reasoning inline, including the #undef trap, so the next person does not have
to rediscover it.

Deliberately not touched

core/cuda_switch.h keeps two families unprefixed, because prefixing them
would destroy the thing they exist to do:

  • #define int8_t … uint64_t under #ifdef __CUDACC_RTC__. NVRTC ships no
    standard library, so these hand definitions are<cstdint> in that mode
    and must keep the standard spellings.
  • #define __device__ / #define __host__ under #ifndef __CUDACC__. These
    erase nvcc's qualifiers for a host compiler; renaming them would leave the
    real names undefined. Correctly guarded — inside #ifndef __CUDACC__,
    and each behind its own #ifndef so a pre-existing definition wins.

core/dlpack.h is vendored upstream (DLPACK_*) and is never rewritten.

Reviewing this

It is one script. Re-derive rather than read:

git checkout HEAD~1 -- include src tests
python3 tools/rename-macros.py
git diff # must be empty

tools/rename-macros.py --check doubles as a lint: it re-scans include/
afterwards and exits non-zero if any unprefixed, non-exempt macro remains. It
currently reports:

include/ is clean: every surviving macro is FF_-prefixed or documented-exempt.

If this needs rebasing (it touches impl/cuda/ widely, so it will conflict
with #86 and #87): do not resolve conflicts by hand. Reset, re-run the script
on the new base, commit that. It is deterministic — that is why it is committed
rather than described. This PR should land after the in-flight CUDA work.

Validation

  • make test CXX=clang++ — CPU suite, 59,886 checks / 13 suites / 0
    failures
    , unchanged. (Results posted below once the clean-worktree run
    finishes.)
  • codespell clean.
  • CUDA compile+link is CI's job; core/ changes trigger the full matrix, which
    is what you want for a rename that touches CUDEV/CUHOSTDEV in 1,500
    places.
  • clang-format will report emptily — see the note in refactor: de-duplicate the dispatch helpers into shared headers #90; that job is broken
    independently of this work and is continue-on-error.

Generated by Claude Code

THE RULE
Every macro that survives preprocessing of a header under include/ -- every
#define not #undef'd before the end of the header that defined it -- must be
spelled FF_*. Installing a header means every translation unit downstream of
it inherits its macros; an unprefixed one is a name this project has silently
taken from everybody who includes us. 184 macros here already followed the
convention. This is the remaining 31, minus the exemptions below.
Macros defined and consumed inside a single .cpp are NOT covered -- they never
leave the translation unit. That is why the dispatch pyramid's local
NDIM_SWITCH / BOUND_SWITCH / MV_ARGS / *_DT / BND1..3 are untouched here:
they are TU-private, and their design is a separate change in other hands.
WHAT CHANGED
FF -> FF_NS (see below)
CUGLOB/CUHOST/CUDEV/CUHOSTDEV -> FF_* core/cuda_switch.h
DISPATCH_PP -> FF_DISPATCH_PP api/{cpu,cuda}/pushpull_dispatch.h
PP_{BOUND,DTYPE,ORDER} -> FF_PP_* (renamed, not redesigned)
ATOMIC_INTEGER_IMPL -> FF_ATOMIC_INTEGER_IMPL
GPU_ATOMIC_INTEGER -> FF_GPU_ATOMIC_INTEGER
INTERPOL_UTILS -> FF_INTERPOL_UTILS
DIST_USE_LOOP -> FF_DIST_USE_LOOP
JFH_OnePlusTiny -> FF_ONE_PLUS_TINY, and de-duplicated: it was #defined
identically in three sibling posdef headers; only
posdef/utils.h keeps it, the other two include it.
uchar_t -> deleted, expanded to `unsigned char` at its six uses.
A lowercase macro impersonating a typedef is worse
than a shouty one: a downstream
`typedef unsigned char uchar_t;` does not merely
collide, it fails with a diagnostic pointing at the
wrong file.
DISPATCH_PP becomes FF_DISPATCH_PP rather than FF_PP_DISPATCH because
impl/cuda/pushpull.h already uses the latter spelling. It #undef's it, so
there is no real collision -- but reusing the name would be needlessly
confusing to read.
WHY FF ITSELF WAS RENAMED
It is a two-letter, all-caps macro in an installed header: the worst offender
on the list, and an entirely plausible downstream identifier. 253 use sites
across 105 files, of which 228 are FF_NAMESPACE_BEGIN(FF)/END(FF).
`#undef FF` at the end of core/defines.h was considered and is not an
alternative -- it is actively wrong. FF is used by ~105 *other* files after
they include defines.h, so undefining it there would turn every
FF_NAMESPACE_BEGIN(FF) into a namespace literally named `FF` rather than into
a compile error. Silently the wrong namespace is worse than the status quo.
Keeping it with a comment was the other option. Rejected: the macro is exactly
the hazard this pass exists to remove, and unlike most renames this one is
verified by the compiler -- a missed site is an undeclared identifier, never a
silent behaviour change. The indirection itself is kept deliberately (headers
must not hard-code `ff::`), so the macro is renamed rather than removed.
DELIBERATELY NOT TOUCHED
core/cuda_switch.h keeps two families unprefixed, because prefixing them would
destroy the thing they exist to do:
* #define int8_t ... uint64_t, under #ifdef __CUDACC_RTC__. NVRTC ships no
standard library, so these hand definitions ARE <cstdint> in that mode.
* #define __device__ / __host__, under #ifndef __CUDACC__ and each behind
its own #ifndef. These erase nvcc's qualifiers for a host compiler;
renaming them would leave the real names undefined.
core/dlpack.h is vendored upstream code (DLPACK_*) and is never rewritten.
The rewrite is tools/rename-macros.py, committed and idempotent. It ends by
re-scanning include/ and failing if any unprefixed, non-exempt macro remains,
so `--check` doubles as a lint. If this needs rebasing, do not resolve
conflicts by hand: reset, re-run the script on the new base, commit that.
The comment explaining why the root-namespace macro was renamed spelled the
old two-letter name literally, which made tools/rename-macros.py non-idempotent
against this one file: a second run rewrote the prose. Restate it using only
the post-rename spelling, so `--check` is clean and the script's idempotency
claim holds. Also expands the reasoning slightly -- the #undef trap is the part
worth not rediscovering.
@balbastyClaude

Copy link
Copy Markdown
CollaboratorAuthor

Two deliberate non-changes, flagged so a reviewer who greps does not read them as misses:

1. MIGRATION-PROVENANCE.md:157 still says CUHOST.

| #47 | _pending_ | Mesh distance: write the missing CUHOST launchers for sdt_naive, udt, udt_naive |

That is a verbatim quotation of the title of fastfields-cuda-impl#47, in the
issue-transfer table. Rewriting it would falsify the record — the issue really
is called that, in a repo this rename does not touch. CLAUDE.md and
MIGRATION.mdwere updated, because there the names describe current code
rather than quote an external title.

The script only walks include/, src/ and tests/ with source extensions,
so the prose files were all handled by hand and each judged individually.

2. typedef double reduce_t; is still repeated 11 times and is left alone
on purpose. It is a genuine duplication, but it is not a naming hazard: it
lives inside ff::cpu / ff::cuda, so it is namespaced and cannot collide
with anything downstream. De-duplicating it would mean opening the device
namespace from a core/ header, which is precisely what core/ must not do —
core/ is backend-agnostic, and ff::<FF_DEVICE> is not. Worth a separate
look if the accumulation type ever becomes configurable; not worth coupling to
this pass.


Generated by Claude Code

@balbastyClaude

Copy link
Copy Markdown
CollaboratorAuthor

Gate: unmoved

test-cpu (clang-dynamic) green on this PR, and test-hub and
compile-probe-cuda with it — so nvcc accepts the renamed
FF_CUDEV / FF_CUGLOB / FF_CUHOST / FF_CUHOSTDEV qualifiers, which is
the half of this change a CPU-only run cannot speak to.

Tail of the CPU job, verbatim:

452/452 checks passed <- solve_field
splinc module CPU tests
checks: 4577, failures: 0
PASSED

Same 13 suites, same 59 886 checks / 0 failures as
tools/test-baseline.expected and as #90. A rename cannot change a count, but
that is the claim, so it is measured rather than assumed.

Why a rename this wide is safe to review quickly

Every one of these is compiler-verified. There is no spelling of this
change that silently does the wrong thing: a missed site is an undeclared
identifier and the build stops. That is the difference between renaming FF
and, say, changing a macro body — and it is the main reason the FF rename was
judged worth its blast radius rather than deferred behind a comment.

The one place that reasoning does not apply is #undef, which is why the PR
body spends a paragraph on it: #undef FF would have compiled fine and put
every symbol in a namespace called FF. That failure mode is exactly the class
this pass exists to eliminate, so it is now written down in
core/defines.h next to the macro.

Merge order

This touches impl/cuda/ and impl/kernels/ very widely (CUDEV alone is 846
sites). It should land after #86 and #87, not before. If it conflicts, do
not hand-resolve:

git reset --hard <new base>
python3 tools/rename-macros.py
git commit -am "refactor: prefix every remaining public macro with FF_"

The script is deterministic and idempotent — --check currently reports
0 file(s) would change — which is the entire reason it is committed rather
than described.


Generated by Claude Code

@balbastyClaude

Copy link
Copy Markdown
CollaboratorAuthor

Local clean-worktree run finished:

$ make -j2 test CXX=clang++ # clean tree, dedicated worktree
MAKE_EXIT=0
13 suites | 59886 checks | 0 failures

And the result that matters most for a rename this wide — the per-suite output
of this branch is byte-identical to #90's:

$ diff <(suite-counts from #90) <(suite-counts from this branch)
(no output)

Same 13 suites, same 59 886 checks, same zero failures, in the same order. A
rename should be invisible to the tests, and it measurably is.

tools/macro-equivalence.py also still reports 0 MISMATCHED when run against
this tree rather than #90's, confirming the prefixing pass did not perturb any
of the de-duplicated macro bodies it inherited.


Generated by Claude Code

distance_mesh.h conflicted with the sdt_naive launcher and vertex-normal
fix from #86. Resolved by taking main's version wholesale and re-running
tools/rename-macros.py over the tree rather than hand-editing: the script
is idempotent and self-verifying, so its clean --check ('0 file(s) would
change', include/ clean) is the evidence the rename is complete on the
new base.
@balbasty
balbasty merged commit f63c7d8 into mainAug 20, 2026
10 of 11 checks passed
@balbasty
balbasty deleted the refactor/prefix-public-macros branch August 20, 2026 01:02
balbasty pushed a commit that referenced this pull request Aug 20, 2026
Brings in #87, #90, #91 and #95. One conflict, in
include/fastfields/impl/kernels/parallel.h: #91 renamed
FF_NAMESPACE_BEGIN(FF) to FF_NAMESPACE_BEGIN(FF_NS) on the line this branch
inserts the FF_GRAIN_SIZE block above. Resolved by keeping both -- the new
block, then main's FF_NS spelling.
Everything else auto-merged. #91's renames do not touch anything this branch
depends on: has_atomic_add / anyAtomicAdd keep their names, FF_NS still
expands to ff, and the CUDEV -> FF_CUDEV rename is confined to the CUDA half
of atomic.h. FF_GRAIN_SIZE, the one macro this branch adds to an installed
header, already satisfies #91's FF_-prefix rule --
`tools/rename-macros.py --check` reports "0 file(s) would change" and
"include/ is clean".
Re-verified on the merge result:
* tools/test-baseline.sh --legs default,lib -> byte-identical to
tools/test-baseline.expected. 13 suites, 59,886 checks, 0 failures.
* -DFF_GRAIN_SIZE=1 -> 59,886 / 13 / 0.
* -DFF_GRAIN_SIZE=1 + TSan, FF_NUM_THREADS=4, halt_on_error=1
-> 59,886 / 13 / 0, zero reports.
* clone syscalls: 0 across all 13 binaries at the shipping grain size,
2 per binary at FF_GRAIN_SIZE=1. The threshold is unchanged by the merge
(0 clones at n=32768, 2 at n=32769, on main and on this branch alike).
* The thread-pool defects still reproduce on main at f63c7d8: the data race
is deterministic under TSan (threadpool.h:148 write / :164 read) and the
lost-wakeup deadlock is stochastic (8/320 trials over FF_NUM_THREADS
8/16/32/64). Both are gone on this branch: 0/320 hangs, 0 TSan reports.
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.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@balbasty@claude