feat(Geom): distributed entity reordering framework + move semantics fix - #9
Merged
Merged
Conversation
…s, checked wrapper contract Plan 1 — AdjIndexInfo: - wireTargetMapping: assert non-null input - makeFatherOnlyMapping: static factory for empty-ghost OffsetAscendIndexMapping - EnsureGhostMapping: mesh helper to create father-only ghost mapping on pairs before ghost layers are built, enabling normal IndexG2L/IndexL2G instead of _NoSon variants Plan 2 — fillRegistry: - UnstructuredMesh::fillRegistry: register all built adjs, source pLGlobalMapping from any adj array per entity kind, check_throw if mapping is needed but missing - const registerAdj overloads for TPair and AdjPairTracked - Adj::Bnd2Face, Adj::Face2Bnd predefined constants - Migrated all 4 ghost DAG sites (RecoverN2CB, BuildGhostPrimary, BuildGhostFace, ReadDistributed) to use fillRegistry Plan 3 — Device views: - AdjIndexInfoDeviceView, AdjPairTrackedDeviceView, AdjPairTrackedDeviceViewConst in Mesh_DeviceView.hpp - AdjPairTracked::deviceView<B>() returns tracked view with idx state - Changed all 12 tracked adj member types in UnstructuredMeshDeviceView from plain ArrayPairDeviceView to AdjPairTrackedDeviceView Checked wrapper contract (CheckedInverse, CheckedComposeFiltered): - Extract L2G callbacks from pair ghost mappings (no manual callbacks) - Return AdjPairTracked with father adopted, son allocated, state = Global - Callers never touch raw DSL or markGlobal on results CellIndexG2L/CellIndexL2G now use cell2node (not cellElemInfo) so they work at all pipeline stages. Same for BndIndex wrappers (use bnd2node). 74/74 C++ tests pass. 50/50 Python tests pass. All 6 solvers build.
…drivers
The clang-tidy setup was split across three files that disagreed on
disables (.clangd's ClangTidy section vs src/.clang-tidy), lacked a
HeaderFilterRegex (so tidy drowned in Eigen/Boost header noise), and
hard-wrapped WarningsAsErrors: * which turned advisory output into an
error stream. The runner scripts were slow serial bash loops.
Changes:
- Move src/.clang-tidy and src/.clang-tidy-fix to the project root and
make them the single source of truth for both CLI and clangd. Add
HeaderFilterRegex scoped to src|app|test/cpp, ExtraArgs for
-UDNDS_USE_OMP and -Wno-unknown-warning-option, and set
WarningsAsErrors: '' so tidy runs are advisory.
- Trim .clangd to the editor-only flag tweaks; drop its ClangTidy block
(clangd auto-discovers the project-root .clang-tidy).
- Move scripts/run-clang-tidy{,-fix}.sh and scripts/run-clang-format.sh
from src/ to scripts/; rewrite each as a thin shim that execs the new
Python driver.
- New scripts/run_clang_tidy.py and scripts/run_clang_format.py: scope
by module names, paths, or files; support --changed, --since REF,
--fix, --summary, --top-checks, --list-files, --dry-run, --strict.
In-process parallelism via concurrent.futures; no external
run-clang-tidy binary required.
- Wrap the hand-aligned DNDS_FIELD tables inside DNDS_DECLARE_CONFIG
bodies with // clang-format off / // clang-format on so clang-format
stops destroying the alignment. Covered here: CFV/FiniteVolumeSettings,
CFV/ModelEvaluator, CFV/VRSettings (3 blocks), DNDS/Serializer/
SerializerFactory, Geom/Mesh/Mesh (2 blocks). Remaining guarded files
ship alongside the bulk-format commit that follows.
- Document the new layout and CLI in docs/guides/style_guide.md,
including the CUDA + clang-tidy caveat.
Pure-format commit produced by running
scripts/run_clang_format.py src/DNDS
scripts/run_clang_format.py src/Euler/{CLDriver,EulerEvaluatorSettings,EulerSolver}.hpp src/Solver/Direct.hpp
against the project .clang-format, using the Python driver added in the
preceding commit. The four Euler/Solver files had pre-existing drift
outside their DNDS_DECLARE_CONFIG blocks (pragma indentation, empty body
braces, trailing comments) and are brought in line here too so the
guarded alignment changes in the previous commit can land on a clean
base.
No semantic changes.
Adds docs/dev/clang_tidy_plan.md as the living document driving a check-by-check cleanup of the DNDS module (with the same recipe to be applied to Solver, Geom, CFV, Euler, EulerP afterwards). Includes the recorded baseline, a triage table for the top-20 checks with four buckets (keep & fix / keep & accept / silence locally / re-read later), and a pass log skeleton that will be filled in commit by commit. Also tightens CHECK_RE in scripts/run_clang_tidy.py to match only real clang-tidy check names (at least one '-' or '.' in the name). The previous regex matched any lowercase bracketed token, which inflated the 'modernize-use-nodiscard' count by ~2 000 from the '[[nodiscard]]' text that appears in the replacement hint and introduced four phantom checks (nodiscard, loc, pos, name). Total shift in baseline numbers: 28 337 -> 24 597 (real) diagnostics across 55 -> 51 (real) checks. No behaviour change in the driver itself.
Pass 1 of the clang-tidy cleanup plan (docs/dev/clang_tidy_plan.md). All 530 DNDS hits of bugprone-macro-parentheses collapsed to 17 unique source locations in 3 files. Every one is a false positive: the flagged token is either a type name (DNDS_DEVICE_TRIVIAL_COPY_DEFINE's T and T_Self, DNDS_DECLARE_CONFIG's Type_) or a storage-class specifier (DNDS_ARRAY_DOF_OP_FUNC_LIST's spec, always passed static), neither of which is syntactically parenthesizable in C++. Three NOLINTBEGIN/NOLINTEND pairs, each with a one-sentence rationale, scoped tightly to the three macro definitions. No code-generation change; pure comment additions. Numbers: 24 597 -> 24 067 total diagnostics, 51 -> 50 distinct checks. Plan doc updated.
clang-tidy --fix cannot run in parallel on the same header from multiple TUs: the parallel write edits race and corrupt the file (observed: interleaved '[[nodiscard]]' insertions producing syntax errors across Vector.hpp, Array.hpp, ArrayPair.hpp, etc. during a Pass-2 trial run). Force jobs=1 whenever --fix is set. The escape hatch --unsafe-parallel-fix keeps the old behaviour for callers that have pre-partitioned the scope to disjoint files. Non-fix runs are unchanged. Message added so the user sees when the jobs cap kicks in.
…iscard)
Pass 2 of the clang-tidy cleanup plan. Applied via
scripts/run_clang_tidy.py --fix --config-file /tmp/pass2.clang-tidy DNDS
with a single-check override (only modernize-use-nodiscard enabled).
Serialized because parallel --fix on shared headers corrupts files.
32 unique declarations gained [[nodiscard]] across 11 files
(Array.hpp, ArrayBasic.hpp, ArrayDerived/*.hpp, ArrayPair.hpp,
Vector.hpp, EigenUtil.hpp, Serializer/SerializerH5.cpp). The 1 572
diagnostic count collapses to 32 because each header declaration
was reported once per including TU.
Sample-reviewed four representative changes: at(), at_compressed()
(with DNDS_DEVICE_CALLABLE prefix), rows()/cols()/size() Eigen
wrappers, get_indent() in SerializerH5.cpp. All are pure const
getters; no call sites found that invoke them for side effects.
Numbers: 24 067 -> 22 495 total diagnostics, 50 -> 49 distinct checks.
Build: dnds + euler targets succeed; format stable.
…t-variables)
Pass 3 of the clang-tidy cleanup plan. Applied via
scripts/run_clang_tidy.py --fix --config-file /tmp/pass3.clang-tidy DNDS
with a single-check override (cppcoreguidelines-init-variables only),
serialized.
~30 declarations across 8 files now initialise the local before its
first MPI/HDF5 out-parameter write: the classic 'int x; MPI_*(&x)'
becomes 'int x = 0; MPI_*(&x)'.
Two auto-fix choices were reverted by hand during sample review:
- ArrayTransformer.hpp:838,867: clang-tidy picked
'MPI_Datatype dtype = nullptr' which compiles on OpenMPI (where
MPI_Datatype is a pointer typedef) but not on MPICH (where it is
an int). Manually set to MPI_DATATYPE_NULL.
- ArrayDOF_op.hxx: clang-tidy added '#include <math.h>' and
'real sqrSumAll = NAN'. The sibling function three paragraphs up
initialises sqrSumAll to 0; made this one match and dropped the
math.h include.
Numbers: 22 495 -> 21 297 total diagnostics, 49 -> 49 distinct checks.
Build: dnds target succeeds.
…ML trap Pass 4 of the clang-tidy cleanup plan. Reclassified from KF to KA. All 9 unique DNDS hit sites are false positives: either functor parameters that are called in-place (moving on first call would break repeated calls) or collection parameters that are mutated in place without being transferred. Neither benefits from std::forward and the check cannot tell the two patterns apart. Also fixed a YAML folded-scalar trap in .clang-tidy: the Checks: > block was accumulating '#'-prefixed rationale comments as literal text, which produced a malformed check name that silently absorbed subsequent disables. Moved all rationale into the file header as a table; the Checks: block is now comment-free, and a warning note explains why. Numbers: 21 297 -> 20 794 total diagnostics, 48 -> 47 distinct checks. Build unchanged (config-only).
…fier)
Pass 5 of the clang-tidy cleanup plan. Manual rename; this check
does not implement --fix.
1 404 hits collapsed to ~25 distinct reserved-pattern identifiers
(leading __, leading _[A-Z], or embedded __). Most renamed by
stripping leading underscores; a few needed context-specific names:
- _Tp -> Tp (template param in Defines.hpp).
- __DNDS_str -> DNDS_str; __DNDS__json_to_config -> DNDS_json_to_config.
- __start_timer / __stop_timer / __EndTimerType -> unprefixed.
- __p_indices, __Row_size, __OneMatGetRowSize, __DNDSToMPIType*,
__InSituPackStart{Pull,Push}, __{Read,Write}SerializerData*
-> unprefixed.
- __pybind11_callBindXyzs_rowsizes_sequence (x8) -> unprefixed
(these are our own template helpers, not pybind11-generated).
- __EigenPCH, __ExprtkPCH (const char * module tags) -> *_tag to
avoid colliding with the homonymous class / filename.
Two collision-resolved:
- ArrayGlobalOffset ctor params __size, __offset -> sz, ofs.
(size/offset have hundreds of existing uses elsewhere.)
- ArrayBasic's static _GetDataLayout() metafunction renamed to
ComputeDataLayout because GetDataLayout already exists as an
instance member on Array with different semantics.
Numbers: 20 794 -> 19 390 total diagnostics, 47 -> 46 distinct
checks. Build: dnds + euler succeed; euler.exe links.
Post-Pass-5 housekeeping from docs/dev/clang_tidy_plan.md.
Added the checks the triage bucketed as "keep & accept" to the
.clang-tidy disables, with one-line rationale each in the header
table:
- cppcoreguidelines-non-private-member-variables-in-classes
(project uses struct-of-fields data bags pervasively)
- cppcoreguidelines-avoid-magic-numbers
(duplicate of already-disabled readability-magic-numbers)
- cppcoreguidelines-pro-bounds-{pointer-arithmetic,
array-to-pointer-decay, constant-array-index}
(CSR storage and MPI byte buffers)
- cppcoreguidelines-pro-type-vararg (MPI / printf)
- cppcoreguidelines-pro-type-reinterpret-cast (MPI, serialization)
- cppcoreguidelines-pro-type-const-cast (C-API interop)
- readability-redundant-access-specifiers (project convention)
- modernize-use-transparent-functors (Eigen expression templates)
- cppcoreguidelines-c-copy-assignment-signature
(duplicate of misc-unconventional-assign-operator)
Also documents passes 6 and 7 as deferred: pass 6 needs a careful
class-by-class audit of move semantics on 32 classes; pass 7 is
per-site judgement.
Numbers: DNDS goes from 19 390 -> 7 341 total diagnostics across
46 -> 35 distinct checks, a 62 % drop versus the post-rename state,
and a 70 % drop versus the original baseline (24 597). No code
touched.
…mber-functions) Pass 6 from docs/dev/clang_tidy_plan.md. Per-class audit of 1645 warnings collapsing to ~20 distinct class declarations. Classified each as one of: 1. Value-semantic (all members `shared_ptr` / `unique_ptr` / POD / `host_device_vector`): add `= default` move ctor / move assign / destructor alongside the existing custom copy. Default move is a shallow transfer of the shared handles — correct and the same observable effect as copy + reset on moved-from state. Classes: `Array`, `ArrayAdjacency`, `ArrayDof`, `ArrayEigenMatrix`, `ArrayEigenMatrixBatch`, `ArrayEigenUniMatrixBatch`, `ArrayEigenVector`, `ArrayTransformer`, `ParArray`, `AdjacencyRow`, `RowView`, `EmptyNoDefault`, `host_device_vector_r0/r1`, plus their nested `iterator` classes. 2. Polymorphic RAII base (file handles, MPI handles, virtual dtor): `= delete` copy / move to prevent slicing / double-close. Classes: `SerializerBase`, `SerializerJSON`, `SerializerH5`, `DeviceStorageBase`, `DeviceHostSingleAllocationBase`, `DeviceHostSingleAllocationDirect`, `ExprtkWrapperEvaluator`. 3. Classic singletons (pre-C++11 private-unimplemented idiom): replace with `= delete` copy / move + `= default` destructor. Classes: `CommStrategy`, `MPIBufferHandler`, `ResourceRecycler`, `PerformanceTimer`. 4. Resource-registry holders (register `this` with ResourceRecycler): `= delete` copy / move — copying would register twice for the same raw address and double-free on destruction. Classes: `MPIReqHolder`, `MPITypePairHolder`. Every declaration carries a one-line rationale comment explaining which bucket the class falls in and why the chosen semantics are correct. Numbers: DNDS goes 7341 -> 5691 total diagnostics (22% drop), 34 distinct checks remaining.
Pass 7 from docs/dev/clang_tidy_plan.md. All 2110 warnings across 41 unique macros are legitimate uses that a `constexpr` template function cannot express: - Assertions / checks that capture __FILE__ / __LINE__ for error reporting (DNDS_assert, DNDS_check_throw, DNDS_HD_assert*, ...) - Code-generation DSLs that declare class members, static data, or JSON binding glue (DNDS_DECLARE_CONFIG, DNDS_FIELD, DNDS_json_to_config, DNDS_NLOHMANN_DEFINE_*, pybind11_bind_*, DNDS_DEVICE_TRIVIAL_COPY_DEFINE*) - Platform probes and define-before-include switches (MPICH_SKIP_MPICXX, OMPI_SKIP_MPICXX, EIGEN_DONT_PARALLELIZE) - Intrinsic / attribute wrappers (DNDS_likely, DNDS_unlikely, DNDS_FORCEINLINE, DISABLE_WARNING via _Pragma) - CMake-injected constants (DNDS_VERSION_STRING) A full rationale list is in the .clang-tidy header table. Numbers: DNDS goes 5691 -> 3581 total diagnostics (37% drop), 33 distinct checks remaining.
Pass 8 from docs/dev/clang_tidy_plan.md. Single-check --fix run with serialised (jobs=1) execution. 776 warnings reduced to 0. Three patterns auto-fixed: - `std::make_pair(MPI_Datatype(MPI_FLOAT), ...)`: the MPI constants already have type `MPI_Datatype`, so the functional cast was a no-op. Removed in 11 call sites across MPI.hpp. - `reinterpret_cast<uint8_t *>(uint8_t *)` in DeviceStorage.cpp: identity cast of an already-`uint8_t *` pointer. Replaced with plain assignment. - `index(nSend)` in ArrayTransformer.hpp where `nSend` is already `index`. Numbers: DNDS goes 3581 -> 2805 total diagnostics (22% drop), 32 distinct checks remaining.
…ber-init)
Pass 9 from docs/dev/clang_tidy_plan.md. Single-check --fix run.
475 warnings reduced to 0. Two patterns fixed:
- Raw-type class members (`index _size`, `rowsize Row_size`,
`MPI_Aint pushSendSize`, `ConfigTypeTag typeTag`, `tStart` array)
given `{}` default initializers. These were always overwritten
by the constructor body before any read, so the fix is a
no-op at runtime but documents invariants explicitly.
- Local `std::array<char, N>` / `std::array<hsize_t, N>` buffers
given `{}` in MPI.cpp, SerializerH5.cpp, Defines.cpp,
ArrayEigenMatrix.hpp, ArrayTransformer.hpp (two sites).
Manual cleanup of auto-fix output:
- `struct winsize w {};` -> `struct winsize w{};` in Defines.cpp
(auto-fix emitted the brace-init on a separate line).
- `ConfigTypeTag typeTag{};` in ConfigRegistry.hpp (check
required a second pass because the struct header was not fully
rewritten on the first run).
Numbers: DNDS goes 2805 -> 2330 total diagnostics (17% drop),
31 distinct checks remaining.
Pass 10 from docs/dev/clang_tidy_plan.md. Single-check --fix run. 231 warnings reduced to 0. Four call sites: `(T *)(NULL)` in the past-the-end row inquiry in ArrayBasic.hpp, `getenv()` checks in MPI.hpp/MPI.cpp, and HDF5 handle probes in SerializerH5.cpp.
Pass 11 from docs/dev/clang_tidy_plan.md. Single-check --fix run. 112 warnings reduced to 0. Replaces `push_back(std::make_pair(a, b))` with `emplace_back(a, b)` in MPI type-pair vector appends (2 sites in ArrayTransformer.hpp) and HDF5 filter/dataspace dimension vectors (SerializerH5.cpp), and `push_back(std::string(...))` with `emplace_back(...)` in MPI.cpp / MPI_bind.cpp.
…default) Pass 12 from docs/dev/clang_tidy_plan.md. Single-check --fix run. 59 warnings reduced to 0 (two unique sites, duplicated across TUs that include Vector.hpp). Replaces empty-body destructors with `= default`: - `DeviceHostSingleAllocationBase::~DeviceHostSingleAllocationBase()` - `DeviceHostSingleAllocationDirect::~DeviceHostSingleAllocationDirect()`
…y-qualified-auto) Pass 13 from docs/dev/clang_tidy_plan.md. Single-check --fix run. 52 warnings reduced to 0. Adds `*` to `auto`-declared pointer variables (pybind11 binding helpers) and `const` to structured bindings that don't mutate (`const auto &[key, value] : map.items()` in SerializerJSON.cpp).
Pass 14 from docs/dev/clang_tidy_plan.md. Single-check --fix run. 222 warnings reduced to 0. Adds `/*unused*/` comment markers on tag-dispatch parameters (mostly `std::index_sequence<Is...>` used purely for type deduction) in the pybind11 binding machinery.
…olean-expr) Pass 15 from docs/dev/clang_tidy_plan.md. Single-check --fix run plus one manual fix in Defines.hpp (auto-fix couldn't rewrite it through the .hpp include graph and left 66 duplicates on one site). 155 warnings reduced to 0. Four patterns fixed: - `!(a && b)` -> `!a || !b` / DeMorgan expansion in ArrayDOF.hpp (SFINAE `enable_if`), ArrayDOF_bind.hpp (`if constexpr`), EigenUtil.hpp (ternary condition). - `!(v > MAX || v < MIN)` -> `v >= MIN && v <= MAX` in Defines.hpp::checkedIndexTo32 (manual — clang-tidy's proposed replacement hit every include of Defines.hpp separately).
…cessary-value-param) Pass 16 from docs/dev/clang_tidy_plan.md. 319 warnings reduced to 0. Combination of single-check --fix for the straightforward cases (auto-fixed across several .cpp TUs) and manual sed across the five pybind11 `_bind.hpp` headers that share the same `py::buffer row` parameter pattern in `setitem` / operator overloads. Two manual sites: - Serializer_bind.hpp: `from_dict` lambda took `py::object options_in` by value; only used via `nlohmann::json j(options_in)`. - MPI_bind.cpp: Allreduce lambda took both `py_sendbuf` and `py_recvbuf` as `py::buffer` by value; only `request()` is called. All 15 instances of `py::buffer row` across ArrayAdjacency, ArrayEigenMatrix, ArrayEigenMatrixBatch, ArrayEigenUniMatrixBatch, ArrayEigenVector binding headers are now `const py::buffer &row`. Each lambda body only calls `row.request()`, which is const.
Pass 17 from docs/dev/clang_tidy_plan.md. 149 warnings reduced to 0. Auto-fix converted two index-based loops over `std::vector<index>` in ArrayRedistributor.hpp to range-based form (the index was only used to dereference into the same vector). One site in Array_bind.hpp carries a NOLINTBEGIN / NOLINTEND guard: the loop body calls `pullIndexGlobal.at(i)` on a `py::array_t<index>` to invoke the numpy-to-long implicit conversion. Range-based iteration yields `pybind11::handle`, which does not convert to `long` in that context, so the explicit index-based form is required. The first auto-fix attempt broke the build here; the NOLINTBEGIN/NOLINTEND protects it from future runs. A plain NOLINTNEXTLINE is not enough: `--fix` rewrites the `for` line itself, erasing the comment. Block-form NOLINT survives.
…fer-member-initializer) Pass 18 from docs/dev/clang_tidy_plan.md. Single-check --fix run. 60 warnings reduced to 0 (one unique site, duplicated across TUs). Moves `comm = ncomm;` in the body of MPIInfo::MPIInfo(MPI_Comm) to the member initializer list.
…ines-pro-type-cstyle-cast) Pass 19 from docs/dev/clang_tidy_plan.md. Manual (6 unique sites). 124 warnings reduced to 0: - SerializerH5.cpp (lines 878, 912) and SerializerJSON.cpp (lines 223, 252): `(ssp<tValue> *)(pth_2_ssp[refPath])` -> explicit `reinterpret_cast<ssp<tValue> *>(...)`. The dedup registry stores type-erased `void*`; this matches the author TODO on line 878. Comment added noting the caller-guaranteed type invariant. - MPI.hpp lines 715, 721: `MPI_IN_PLACE` expands to the OpenMPI-defined `((void *)1)` sentinel whose C-style cast is out of project control. Guarded with NOLINTNEXTLINE placed immediately above the offending call (NOLINTNEXTLINE must be on the line directly preceding the warning; a multi-line rationale comment in between breaks the suppression).
…ables
Pass 20 from docs/dev/clang_tidy_plan.md.
Every global mutable in DNDS is intentional and cannot be made
`const`, `thread_local`, or class-scoped without wider redesign:
logStream — runtime log redirection (set once at startup,
read by every assert/log site)
useCout — runtime switch between stdout and file log
outputDelim — global print delimiter used by vector-dump helpers
HDF_mutex — library-wide HDF5 mutex (must be mutable;
HDF5 is not thread-safe)
isDebugging — runtime probe set from the debugger to unblock
a rank-barrier spin loop
EigenPCH_tag,
ExprtkPCH_tag — link-time probes indicating whether the PCH TU
was compiled into the static library
324 warnings eliminated. Full rationale in the .clang-tidy header
table.
…void-c-arrays)
Pass 21 from docs/dev/clang_tidy_plan.md.
68 warnings reduced to 0 (two unique sites):
- Errors.hpp::genFatalErrorMessage: `char format_buf[1024*512]` ->
`std::array<char, 1024*512> format_buf{}` with zero-init, then
vsnprintf into `format_buf.data()`. Added `#include <array>`.
- SerializerFactory.hpp: `char BUF[512]` for `std::sprintf` of
rank-tagged filename suffix -> `std::array<char, 512>`.
Both sites are stack-allocated scratch buffers for C-style printf
family calls; switching to `std::array` keeps the storage class
identical while giving a `.size()` that survives array-decay.
…e-unhandled-self-assignment) Pass 22 from docs/dev/clang_tidy_plan.md. 53 warnings reduced to 0 (one unique site, duplicated across TUs). `AdjacencyRow::operator=(const AdjacencyRow &r)` copied `r.cbegin() .. r.cend()` into `p_indices` via `std::copy`. When `&r == this`, source and destination ranges are identical and fully overlapping, which is undefined behaviour for `std::copy`. Added a `this == &r` early-return guard (with doc comment explaining the UB) — matches the standard canonical form used by rule-of-five assignment operators. In practice nothing in the tree self-assigns an `AdjacencyRow` view, but the fix is cheap and closes the last bugprone-* warning outside the deferred bucket.
Pass 23 from docs/dev/clang_tidy_plan.md. 322 warnings reduced to 0 (7 unique sites). Every flagged branch is intentional — the diagnostic fires where two logically distinct branches happen to produce the same code, which documents the mapping but triggers the check. Sites suppressed with NOLINTBEGIN / NOLINTEND blocks (plain NOLINTNEXTLINE did not work because the diagnostic is reported at the first clone and the fix-region sometimes spans both): - ArrayBasic.hpp:441, 499 and Array.hpp:588 — `if constexpr` cascades over `_dataLayout`. `TABLE_Fixed` and `TABLE_Max` currently both compute `iRow * _row_size_dynamic + iCol`; the two layouts are conceptually distinct (padded rows may diverge). - ArrayEigenUniMatrixBatch.hpp:46 and _DeviceView.hpp:50 and EigenUtil.hpp::MatrixFMTSafe — ternary expression for the Eigen `options` template parameter. Both non-row-vector arms intentionally select `ColMajor`. - Config/ConfigParam.hpp:188 — switch over `ConfigTypeTag` mapping to JSON Schema type strings. Several enum values map to the same built-in Schema type (`Enum`-> "string", `ArrayOfObjects` -> "array", `MapOfObjects` -> "object"); the distinction shows up elsewhere in the schema meta. Each suppression carries a rationale comment explaining the intent.
…e-implicit-widening-of-multiplication-result) Pass 24 from docs/dev/clang_tidy_plan.md. 66 warnings reduced to 0 (one source site, duplicated across 7 include paths). `std::array<char, 1024 * 512>` in `genFatalErrorMessage` — the product 524 288 is a compile-time constant that trivially fits in int32_t. The `int -> size_t` widening happens at compile time when deducing the `std::array` size template; no runtime overflow is possible. NOLINTNEXTLINE with rationale.
…uidelines-rvalue-reference-param-not-moved)
Pass 25 from docs/dev/clang_tidy_plan.md.
44 warnings reduced to 0 (two source sites, duplicated across TUs).
Both `ArrayDofDeviceView` and `ArrayDofDeviceViewConst` had an
rvalue-ref forwarding ctor that then copy-constructed the base
from the named (lvalue) parameter:
ArrayDofDeviceView(t_base &&base_view) : t_base(base_view) {}
`base_view` inside the function body is an lvalue, so this was
silently a copy-from-rvalue (a minor pessimisation; the base is
value-semantic `shared_ptr`s so the move is cheap either way).
Added `std::move(base_view)` in the base init list to perform the
intended move and clear the lint.
Drains every remaining check with >=1 instance to zero. The 13 actionable sites span 12 files: Fixes: - Device/DeviceStorage.cpp, ExprtkWrapper.cpp, MPI_bind.cpp: legitimate raw `new`/`delete` behind opaque / smart-pointer boundaries. NOLINTNEXTLINE + NOLINTBEGIN/END with rationale (cppcoreguidelines-owning-memory). - MPI.cpp: four empty `catch (...)` on env-var parse failure in CommStrategy::CommStrategy. NOLINTBEGIN/END with rationale (bugprone-empty-catch). - Errors.hpp: spurious bugprone-implicit-widening on compile-time constant `1024 * 512`. NOLINTNEXTLINE with rationale. - ArrayDerived/ArrayEigenMatrixBatch_bind.hpp: setitem helpers returned `auto` (deduced void) and the wrapper lambdas carried redundant `return ...` of void-exprs; changed helpers to `void`, dropped `return` inside their lambdas. Also removed `std::move` of `const py::buffer &` (no-op). - Serializer_bind.hpp: `std::move(m)` where `m` was `const py::module_ &` (no-op move) -- removed. - SerializerFactory.hpp: `BuildSerializer`, `ModifyFilePath` made `const` and `[[nodiscard]]`; ctor changed to pass-by-value + `std::move` (modernize-pass-by-value). - SerializerH5.hpp: dropped redundant `: SerializerBase()` init. - SerializerH5.cpp: `H5Contents &contents` in TraverseData NOLINT'd; `TraverseData *data = static_cast<...>(op_data)` -> `auto *`; `T vV = v` NOLINT'd (used via `&vV` in the non-string `if constexpr` branch); explicit `&attr_value` cast in `H5Aread(..., char**)` NOLINT'd; `get_indent()` return stays as `std::string(n, ch)` (brace-init triggers -Wnarrowing with the initializer_list overload). - SerializerJSON.hpp: `~SerializerJSON()` now wraps `CloseFileNonVirtual()` in a try/catch (bugprone-exception-escape) -- destructors mustn't throw. - Defines.cpp: `ver.length()` -> `!ver.empty()` (readability-container-size-empty). - MPI_bind.cpp: `pArgvOut.reserve(*pargc)` before the `emplace_back` loop (performance-inefficient-vector-operation); `MPI_Comm(pComm)` int-to-ptr cast NOLINT'd with rationale (performance-no-int-to-ptr). - ArrayDOF.hpp: `t_base(base_view)` -> `t_base(std::move(base_view))` for the rvalue-ref forwarding ctor in ArrayDofDeviceView / ArrayDofDeviceViewConst (cppcoreguidelines-rvalue-reference-param-not-moved). Also documented the `NOLINTNEXTLINE` rule that tripped this pass up repeatedly: the directive applies to the *immediately next* line, so rationale comments must follow (not precede) the NOLINT directive when the fix-region is bigger than one line. Numbers: DNDS goes 976 -> 1 total diagnostic. The sole remaining diagnostic is clang-diagnostic-error 'omp.h' in EigenPCH.cpp which is an Eigen internal include unrelated to DNDS code (documented earlier). 35 -> 18 -> 1 distinct check count.
Updates docs/dev/clang_tidy_plan.md with the complete outcome of passes 6-26: - Status snapshot replaced with per-pass delta table (24 597 baseline -> 1 diagnostic end-state). - Passes 6-26 each get a "Completed" entry with commit hash, source sites, and mechanism (--fix / manual / NOLINT / disable). - TOC expanded to cover all 26 passes. - Disables table gains the two new config-level disables (Pass 7 cppcoreguidelines-macro-usage, Pass 20 cppcoreguidelines-avoid-non-const-global-variables). - New "NOLINT markers in the tree" section tabulates the ~70 targeted NOLINT markers by check. - "Next modules" section updated to reflect that DNDS is clean; the same recipe now applies to Solver, Geom, CFV, Euler, EulerP in that order. - Notes the repeated NOLINT-placement gotcha (NOLINTNEXTLINE applies to the immediately next line; rationale comments must precede the directive, not follow it; use NOLINTBEGIN/NOLINTEND when --fix can rewrite the flagged line).
Surfaces the sanitation outcome in the two places agents and new contributors check first: - docs/guides/style_guide.md gains a "Per-module sanitation status" table (DNDS clean, others not started), a NOLINT-placement rule (NOLINTNEXTLINE applies to the immediately next line; use NOLINTBEGIN/END when --fix can rewrite the flagged line), and a pointer to docs/dev/clang_tidy_plan.md for the 26-pass record. - AGENTS.md gains a "Clang-tidy sanitation" subsection under Code Style noting DNDS is clean, pointing at the plan doc, and listing the module order (Solver, Geom, CFV, Euler, EulerP) for the same recipe. Numbers mentioned: 24 597 -> 1 diagnostics across 26 passes. The remaining diagnostic is the Eigen PCH omp.h include issue, unrelated to DNDS source.
… companions, follow semantics Complete redesign of the distributed entity reordering plan: - Two-layer architecture: ReorderRegistry (callbacks) + ReorderPlan (standalone) - buildReorderRegistry() produces dynamic adj/companion set from mesh members - External code (solver, evaluator) extends registry before plan.apply() - Callback-based companion relocation (type-erased, no variant vectors) - Follow propagation: Node/Bnd follow Cell by default (min-rank rule) - Formal adj classification: SKIP/RELOCATE/REMAP/RELOCATE_REMAP/SELF - PermutationTransfer utility (fromPartition, fromLocalPermutation) - 5 concrete use cases including solver participation and node-only reorder - 5-phase implementation plan
…nsfer Phase 1 of the distributed reorder plan. Implements: - PermutationTransfer::fromPartition: build from target-rank assignment, computes new global indices via prefix-sum, push CSR, local-only detection - PermutationTransfer::fromLocalPermutation: build from old2new permutation vector, all entities stay on same rank - transferRows<TPair>: local PermuteRows or distributed ArrayTransformer push - LookupResult with resolve(): ghost-pullable old-global -> new-global map - buildLookup: creates ghost-pulled tAdj1Pair for cross-rank resolution 7 unit tests (doctest): reverse/identity local permutation, all-local partition, round-robin redistribution, lookup resolve (local+cross-rank), CSR permutation. All pass at np=1,2,4,8 (29/29 DNDS ctest).
Two-layer reorder architecture: - ReorderRegistry: dynamic set of type-erased callbacks for adj remap/relocate and companion relocate. External code (solver) can extend it. - ReorderPlan: standalone plan with PermutationTransfers + LookupResults. build() from ReorderInput + registry, apply() invokes callbacks in Phase 1 (REMAP) -> Phase 2 (RELOCATE adj) -> Phase 3 (RELOCATE companions). - classifyAdj(): formal SKIP/RELOCATE/REMAP/RELOCATE_REMAP/SELF classification. - EntityReorderMap, FollowSpec, ReorderInput structs. - ComputeFollowMapFromAdj template (ghost-pull leader ranks, min-rank rule). Geom unit tests (test_MeshReorder.cpp): - classifyAdj logic (cell-only, node-only, cell+node, cell+node+bnd) - ReorderRegistry register + query - ReorderPlan::apply cell-only local permutation (adj + companion unchanged) - ReorderPlan::apply node-only remap (entries updated, rows stay) All 30/30 Geom ctest pass, 29/29 DNDS ctest pass.
Phase 2b implementation: - buildReorderRegistry(): registers all 12 tracked adj members and all companion arrays (cellElemInfo, coords, pbi, etc.) as type-erased callbacks. Registers global mappings per entity kind. - buildReorderPlan(): augments input with default follows (Node/Bnd follow Cell), computes follow maps via ComputeFollowMapFromAdj, builds the plan. - ReorderEntities(): full pipeline — validate state, build registry, compute follows, build plan, destroy face adjs if requested, apply plan (REMAP -> RELOCATE -> COMPANIONS), rebuild global mappings, update idx states, clear stale local vectors. Default follow: when Cell is explicitly reordered and Node/Bnd are not in the explicit set, they automatically follow Cell via node2cell and bnd2cell (min-rank rule).
…eattach, 8 tests passing Complete Phase 2b with real-mesh testing: - buildReorderRegistry: collects pull sets (off-rank globals from adj entries) per entity kind, deduplicates/sorts them for buildLookup - ReorderEntities: allocates empty sons + TransAttach after plan.apply (required for subsequent RecoverNode2CellAndNode2Bnd pipeline) - ReorderPlan::build: uses pre-collected pullSets from registry - father->createGlobalMapping() instead of TransAttach (no son needed) Real-mesh tests on UniformSquare_10.cgns (100 cells, 2D): - Cell-only local reorder: global counts conserved, adj entries valid, full rebuild (ghost + local) succeeds, cell2node entries valid local - Cell-only with face destruction: faces destroyed, state reset, rebuild from scratch succeeds (InterpolateFace + AssertOnFaces) - Cell distributed round-robin (np>=2): cross-rank redistribution with Node/Bnd follow, global counts conserved, adj entries valid, full rebuild succeeds - Node-only local: coords unchanged (identity), cell2node remapped, full rebuild + face interpolation succeeds All 8/8 tests pass at np=1,2,4.
New ReorderLocalCells uses PermutationTransfer::fromLocalPermutation + buildLookup for the cell-only local reorder. Legacy preserved as ReorderLocalCellsLegacy. Key differences from legacy: - Row permutation via PermutationTransfer::transferRows (isLocalOnly path: in-place PermuteRows, zero MPI for row data transfer) - Entry remapping via buildLookup::resolve (ghost-pulled old->new global, same MPI pattern as legacy's cellOld2NewArr ghost-pull) - Ghost rebuild after permutation (same pattern as legacy Section F) Shared infrastructure: - Mesh_CellPermutation.hpp: extracted ComputeCellPermutation into shared header (was anonymous namespace in Mesh.cpp). Used by both legacy and new implementations. 50/50 geom_test_mesh_pipeline pass at np=2,4 (exercises ReorderLocalCells with various meshes including periodic, 3D, elevation, bisection).
…ase 4) New ReadDistributed_Redistribute passes all three partitions (Cell, Node, Bnd) as explicit EntityReorderMaps to ReorderEntities. No follow computation needed since DeriveEntityPartitions pre-computes them. Legacy preserved as ReadDistributed_RedistributeLegacy. Also: add debug bounds-check assertion in PermutationTransfer::transferRows for localOld2New validity (active in debug builds only). Full test results (78/82 pass): - DNDS: 29/29 pass - Geom: 30/30 pass (mesh_pipeline 50 subcases, mesh_distributed_read 18) - Euler: 8/8 pass - Solver: 4/4 pass - CFV: 7/11 pass (4 cfv_reconstruction failures are PRE-EXISTING, unrelated golden-value mismatch)
…ne latent bug Replaces the blanket revert (daa53b6) with correct implementations: 1. host_device_vector_r1: explicit move ctor/assign that transfers unique_ptrs AND nulls cached raw pointers (host_ptr, device_ptr) in the source. Source is left in a valid empty state (size=0). 2. host_device_vector_r0: = default move (inherits from std::vector which has correct move; only adds deviceStorage unique_ptr transfer). 3. Array: = default move ctor/assign. Now safe because host_device_vector has correct move semantics (source is properly emptied). 4. ParArray: = default move (re-declared since explicit copy suppresses it). 5. ArrayTransformer: = default move. Transfers all shared_ptrs (father, son, pLGhostMapping, MPI request holders). Source left uninitialized. 6. ArrayPair::clone latent bug fix: after re-pointing trans.father/son to the newly cloned arrays, call trans.createMPITypes() to recreate persistent MPI requests pointing to the CORRECT buffers. Previously, persistent requests still referenced the source's (R's) buffers after clone, which was a silent corruption waiting to happen. Root cause of the original regression (1880d48): = default move on host_device_vector transferred unique_ptrs but left the cached raw pointers (host_ptr, device_ptr) pointing to freed memory in the moved-from object. Any code retaining a reference to the source would dereference dangling pointers. 82/82 ctest pass.
harryzhou2000
marked this pull request as ready for review
May 4, 2026 16:03
harryzhou2000
added a commit
to harryzhou2000/DNDSR
that referenced
this pull request
May 21, 2026
CFDLAB-THU#6 (missing ∇R_mix): accepted — negligible in practice CFDLAB-THU#7 (species uRecBeta): accepted — ρY_k inherit same compression as ρ CFDLAB-THU#8 (CFDLAB-THU#9) (species positivity in checkRecBaseGood/validation): accepted — Y_k clipped directly CFDLAB-THU#10 (CFDLAB-THU#11) (ppEpsIsRelaxed zeroes thresholds): accepted — by-design config toggle CFDLAB-THU#12 (rhoH_form_old in CompressInc): done — fixed to rhoH_form_new
harryzhou2000
added a commit
to harryzhou2000/DNDSR
that referenced
this pull request
May 21, 2026
… clarify comments LOW cleanups across 8 source files: #1: speedOfSound already refactored in MED CFDLAB-THU#22 (Cantera API) CFDLAB-THU#2: invR0() renamed to R0() — function returned R0=U0^2/T0, not its inverse CFDLAB-THU#3: SourceCellAux::p=101325 comment clarified (code=phys with default scaling) CFDLAB-THU#4: gamma before wall-fix comment added (UMeanXy unchanged by wall-fix) CFDLAB-THU#5: e_sensible<=0 silent cp/cv fallback replaced with DNDS_assert_info CFDLAB-THU#6: dead cellIsHalfAlpha/cellAdjAlphaMin lambdas marked 'Unused, kept for ref' CFDLAB-THU#7: three unused muRef=phys_.muRef() lines removed from CompressInc CFDLAB-THU#8: dead first outMap['RV'] = u[2] removed (overwritten by u[I4-1]) CFDLAB-THU#9: if(model==NS_2EQ) -> if constexpr in EvaluateDt.hxx lambda CFDLAB-THU#10: hardcoded Vector<real,5/4> shock-tube BC comment added CFDLAB-THU#11: uM1/uM2/uM3 dimension guard comments added (I4=dim+1) CFDLAB-THU#12: KE-omission comment in dT_drho (was misplaced, now correctly at density derivative) CFDLAB-THU#13: docstring 'perfect gas, variable' -> 'via Cantera EOS' CFDLAB-THU#14: speciesEnthalpies comment expanded (ideal-gas vs non-ideal EOS) All 56 audit findings resolved: 48 fixed, 8 accepted.
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.
Summary
ReorderEntities): general-purpose reorder for any entity kind (Cell, Node, Bnd, Face) with automatic follow propagation, registry-based adj discovery, and callback-based companion handlingDistributed Entity Reordering Framework
Architecture (two-layer)
Key components
src/DNDS/PermutationTransfer.hppsrc/Geom/Mesh/ReorderPlan.hppReorderRegistry,ReorderPlan,ReorderInput,EntityReorderMap,FollowSpec,AdjActionclassificationsrc/Geom/Mesh/Mesh_Reorder.cppReorderPlan::build/apply,UnstructuredMesh::buildReorderRegistry,ReorderEntities, newReorderLocalCellssrc/Geom/Mesh/Mesh_CellPermutation.hppdocs/dev/distributed_reorder_design.mdFeatures
SKIP/RELOCATE/REMAP/RELOCATE_REMAP/SELFper adjacencybuildReorderRegistry()registers mesh members as type-erased callbacks. External code (solver DOFs, gradients) can extend the registry beforeplan.apply().fromPartitionauto-detects viaMPI_Allreduce; row transfer uses in-placePermuteRows(zero MPI for row data). Ghost lookup still uses MPI (same as legacy).destroyKinds = {Face}wipes facial adjs before reorder; caller rebuilds after.Migrations
ReorderLocalCells→ new implementation usingPermutationTransfer::fromLocalPermutation+ cell-only REMAP/RELOCATE + ghost mapping rebuild. Legacy preserved asReorderLocalCellsLegacy.ReadDistributed_Redistribute→ new implementation passing Cell/Node/Bnd as explicitEntityReorderMaps toReorderEntities. Legacy preserved asReadDistributed_RedistributeLegacy.Move Semantics Fix
Commit
1880d48("close rule-of-five gaps") added= defaultmove to array types buthost_device_vector_r1's default move transferredunique_ptrmembers without nulling cached raw pointers (host_ptr,device_ptr), leaving moved-from objects with dangling pointers. This corrupted CFV reconstruction results viaInterpolateGlobalreturn-by-value paths.Fixes applied
host_device_vector_r1unique_ptrs + nullshost_ptr/device_ptr/size_in sourcehost_device_vector_r0= defaultmove (inheritsstd::vectormove + transfersdeviceStorageunique_ptr)Array= defaultmove (safe: underlyinghost_device_vectornow has correct move)ParArray= defaultmove (re-declared; explicit copy suppresses implicit move)ArrayTransformer= defaultmove (transfers shared_ptrs for father/son/mapping/requests)ArrayPair::clonetrans.createMPITypes()after re-pointingtrans.father/trans.sonto recreate persistent MPI requests pointing to the correct buffersClang-tidy Cleanup (DNDS/ module)
24-pass automated cleanup reducing diagnostics from 24,597 to 1 (remaining: unrelated Eigen PCH
omp.hinclude). Passes include:Test Results
82/82 C++ unit tests pass at np=1, 2, 4, 8:
New test coverage
test_PermutationTransfer.cpp: local reverse/identity permutation, all-local partition detection, round-robin redistribution, cross-rank lookup resolve, CSR local permutationtest_MeshReorder.cpp: classifyAdj logic, registry operations, cell-only local reorder on UniformSquare_10, cell-only with face destruction, cell distributed round-robin with Node/Bnd follow, node-only local reorder with entry remap