Skip to content

Dependency / header-hygiene review: discussion of remaining findings #127

@grzanka

Description

@grzanka

Context

This issue collects the remaining findings from a dependency-structure and header-hygiene review of libdedx, grouped into one place so we can discuss tradeoffs and decide what (if anything) to act on before opening focused PRs. The dedx_const.h dead-code finding is tracked separately in #126.

TL;DR of the overall review (the good news first)

The dependency architecture is fundamentally sound:

  • No circular dependencies. The full header→header graph is a clean DAG. Hub headers are dedx.h (included by 6 headers) and dedx_elements.h (4) — exactly what you'd expect for core type headers.
  • Include guards are correct and consistent. All 21 internal + 16 embedded headers use #ifndef/#define/#endif; every guard macro matches its #ifndef, and all 37 macro names are unique.
  • Clean public/private split. Public include/ headers never pull in a src/ header.
  • Headers are self-contained for std types (bool/size_t/FILE/intN_t each pull their own <std*.h>).
  • Modern CMake — object library feeding static + shared, target_include_directories with PUBLIC/PRIVATE + $<BUILD_INTERFACE>/$<INSTALL_INTERFACE>, namespaced dedx:: exports.

So the items below are refinements, not fixes for anything broken. Listed roughly by impact.


1. static const data arrays defined in headers (highest impact)

src/dedx_periodic_table.h defines data, not just declarations:

static const float dedx_amu[112]  = { ... };
static const int   dedx_nucl[112] = { ... };

This header is #included by 4 translation units: dedx.c, dedx_wrappers.c, dedx_validate.c, dedx_periodic_table.c. Because the arrays are static, each TU gets its own private copy baked into its object file — duplicated data and a latent footgun if anyone ever assumes shared state.

src/dedx_program_const.h is the same pattern at larger scale — 7 static const tables, including char[1000][40] and char[120][40] (tens of KB). Today it's included by only 1 TU (dedx.c), so there's no duplication yet — but the moment a second file includes it, the bloat is silent.

Best-practice shape: declare extern const in the header, define once in a .c:

/* dedx_periodic_table.h */
extern const float dedx_amu[112];
extern const int   dedx_nucl[112];

/* dedx_periodic_table.c */
const float dedx_amu[112]  = { ... };
const int   dedx_nucl[112] = { ... };

Discussion points:

  • Any reason these were intentionally static in headers (e.g. to let the compiler constant-fold / inline lookups, or a past single-TU assumption)?
  • Worth measuring the actual .so/static-lib size delta before/after — for WASM builds in particular, dead-stripping may already hide most of the cost.
  • If we change dedx_program_const.h, do we want to keep it header-only with inline-style accessors instead, or commit to the extern + .c split for consistency with the rest of the codebase?

2. No extern "C" guards in the public headers (API correctness)

None of the installed public headers — include/dedx.h, dedx_tools.h, dedx_wrappers.h, dedx_elements.h, dedx_error.h — wrap declarations in:

#ifdef __cplusplus
extern "C" {
#endif
...
#ifdef __cplusplus
}
#endif

These headers are installed and exported via the CMake package, so a C++ consumer that #includes them gets name-mangled declarations and link errors unless they wrap the include themselves.

Discussion points:

  • Are there any C++ consumers today, or is the consumer set effectively WASM/JS + Python ctypes (where this doesn't matter)? The change is cheap and harmless either way, so the question is just whether it's worth the churn now vs. when a C++ consumer actually appears.
  • If we add them, do it for all 5 installed headers in one pass for consistency.

3. Orphaned embedded data headers (dead artifacts?)

Three generated data headers exist but are never #included anywhere:

  • src/data/embedded/dedx_estar.h
  • src/data/embedded/dedx_icru90_e.h
  • src/data/embedded/dedx_icru90_pos.h

These look like electron / positron tables. They're either leftovers from the data generator or features that were never wired into dedx_embedded_data.c.

Discussion points:

  • Are electron/positron (ESTAR / ICRU90 e⁻ / e⁺) tables a planned feature? If yes, this is a "wire it up" task; if no, they're dead generator output that should be dropped (or excluded from the generator) to avoid confusion.

4. Minor / stylistic

  • Include-guard style: everything uses classic #ifndef guards (portable, standard, consistent). I'd keep this rather than switch to #pragma once. Noting it only because "modern guards" sometimes means #pragma once — flagging that the project deliberately doesn't use it, in case there's a preference to revisit.
  • Test include path is over-broad: tests/CMakeLists.txt adds src/ to the include path for all test targets, though only the white-box tests (test_validate_internal.c, test_core_api.c, which include dedx_validate.h / dedx_mpaul.h) actually need internal headers. Pragmatic as-is; the only downside is that any test can accidentally couple to internals. Could scope src/ to just those targets if we want stricter encapsulation.

Proposed disposition (for discussion)

# Finding Suggested action
1 static const arrays in headers Refactor to extern const + single .c definition (start with dedx_periodic_table.h) — pending agreement on rationale
2 Missing extern "C" guards Add to all 5 public headers if any C++ consumption is anticipated
3 Orphaned embedded headers Decide: wire up e⁻/e⁺ data, or remove
4 Test include path / guard style Likely leave as-is; documented for completeness

Happy to open focused PRs for whichever of these we agree on. cc maintainers for input on #1's rationale and #3's roadmap intent.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions