Skip to content

test(meta): raise meta coverage to >=90% + fix ffi CallbackRegistry crash#4

Open
MaxQian888 wants to merge 23 commits into
ElementAstro:devfrom
MaxQian888:dev
Open

test(meta): raise meta coverage to >=90% + fix ffi CallbackRegistry crash#4
MaxQian888 wants to merge 23 commits into
ElementAstro:devfrom
MaxQian888:dev

Conversation

@MaxQian888

@MaxQian888 MaxQian888 commented Jun 11, 2026

Copy link
Copy Markdown

Summary

Brings the atom/meta test suite to comprehensive, 1:1-with-headers coverage and fixes two latent bugs in ffi.hpp uncovered along the way.

Tests

  • Add the previously-missing tests/meta/reflection/test_refl_field.cpp (covers FieldBase: construction/defaults, member-pointer binding, validators, and the C++23 deducing-this builders incl. derived-type preservation).
  • Expand the meta test suites so every header with runtime code reaches ≥90% deduplicated source-line coverage (33 of 34 at 92.5%–100%). Coverage was measured per-test to avoid gcov's per-instantiation line inflation, which otherwise makes template headers report 5–15× their real line count.
  • facade_any stops at its reachable ceiling (~88%): the remaining lines are defensive proxy-call catch blocks and the never-invoked less_than_impl, unreachable without fault injection.
  • Full suite: 1233 test cases, 0 failures across all 37 meta test executables.

ffi fixes

  • CallbackRegistry SIGSEGV: registerCallback stored the raw closure type in std::any while getCallback<Sig> retrieved it as std::function<Sig>; the pointer-form any_cast returned nullptr on the mismatch (it does not throw, so the surrounding catch was dead code) and the null was wrapped in a success result, so callers dereferenced null and crashed. Now the callable is wrapped in a std::function with the deduced signature (sync + async paths), and getCallback null-guards to return TypeMismatch.
  • Lazy getHandle: DynamicLibrary::getHandle now honours the load strategy like getFunction (Lazy/Immediate load on access; OnDemand still needs an explicit loadLibrary()).

Note on scope

This branch also carries 6 pre-existing branch-sync / pre-commit.ci bot commits that were already on local dev but not yet on upstream dev; they appear in the diff but are not part of the substantive change above.

🤖 Generated with Claude Code


Summary by cubic

Raises atom/meta runtime-header coverage to ≥90% and fixes an ffi callback crash. Also completes the components event system and module macros, consolidates redundant “advanced_*” code (notably the system/command executor), reduces logging noise, and updates tooling.

  • New Features

    • Added tests/meta/reflection/test_refl_field.cpp and split shared field logic into atom/meta/refl_field.hpp.
    • Completed the components event system with Event, EventCallback, EventCallbackId in core/types.hpp; enabled events by default; added Registry::registerComponentInstance.
  • Refactors and Fixes

    • FFI: CallbackRegistry now stores callbacks as std::function<Sig> and returns TypeMismatch on signature mismatch; DynamicLibrary::getHandle respects the load strategy.
    • Consolidated “advanced_*” variants: components bindings moved to scripting/bindings, and system/command’s advanced executor merged into executor with a new command/command.hpp umbrella.
    • Fixed module macros and re-enabled types/macros tests; demoted hot-path logging to trace/debug; formatted exception messages via fmt::format; removed dead components/data/type_conversion.hpp and the Abseil dependency from package.hpp.
    • Tooling/docs: bumped pre-commit-hooks to v6.0.0, added WARP.md, and applied formatting-only fixes.

Written for commit b2682aa. Summary will update on new commits.

Review in cubic

pre-commit-ci Bot and others added 8 commits August 11, 2025 18:16
updates:
- [github.com/pre-commit/pre-commit-hooks: v4.6.0 → v6.0.0](pre-commit/pre-commit-hooks@v4.6.0...v6.0.0)
updates:
- [github.com/pre-commit/pre-commit-hooks: v4.6.0 → v6.0.0](pre-commit/pre-commit-hooks@v4.6.0...v6.0.0)
Add the previously-missing test_refl_field.cpp covering FieldBase:
construction/defaults, member-pointer binding, validators, and the C++23
deducing-this builders including derived-type preservation through
chaining.

Expand the meta test suites so every header with runtime code reaches at
least 90% deduplicated source-line coverage (measured per-test to avoid
gcov's per-instantiation line inflation). facade_any stops at its
reachable ceiling: the remaining lines are defensive proxy-call catch
blocks and the never-invoked less_than_impl, which cannot be reached
without fault injection.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
CallbackRegistry stored the raw closure type in std::any while
getCallback<Sig> retrieved it as std::function<Sig>. The pointer-form
any_cast then returned nullptr on the type mismatch (it does not throw,
so the surrounding catch was dead code), and getCallback wrapped that
null pointer in a success result, so callers dereferenced null and
crashed with SIGSEGV.

Store the callable wrapped in a std::function with the deduced signature
for both the sync and async paths (a private registerAsyncImpl recovers
R(Args...) so the async entry is std::function<future<R>(Args...)>), and
null-guard getCallback so a signature mismatch returns TypeMismatch.

Also make DynamicLibrary::getHandle honour the load strategy like
getFunction: Lazy and Immediate load on access, OnDemand still requires
an explicit loadLibrary().

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @MaxQian888, your pull request is larger than the review limit of 150000 diff characters

pre-commit-ci Bot and others added 15 commits June 11, 2026 03:44
Survey of atom/components found namespace chaos, a dead event system
referencing undefined types, broken module macros, 6 disabled test
files, trait/JSON duplication vs atom/meta and atom/type, and CMake
debt. Spec defines 7 incremental work packages keeping the 332-test
baseline green.

Co-Authored-By: Claude Fable 5 <[email protected]>
14 tasks covering the 7 work packages from the design spec, each
gated on rebuild + test run against the 332-test baseline.

Co-Authored-By: Claude Fable 5 <[email protected]>
Fix mis-indented reset()/updateExecutionTime()/legacy getter bodies and
drop the MSVC-vs-GCC constexpr fork: atomics cannot be constexpr-reset,
so a single plain noexcept variant is correct on all compilers.

Co-Authored-By: Claude Fable 5 <[email protected]>
…dex_sequence

The 98-line manual arity-0..8 if-constexpr ladder, textually included
inside Component::def via #include, is replaced by a single
std::index_sequence generic-lambda expansion over FunctionTraits.

Co-Authored-By: Claude Fable 5 <[email protected]>
…d code

OP_*/CONDITION_*/REGISTER_OPERATOR leaked from the public header into
every consumer. registerOperators now uses std::equality_comparable /
std::totally_ordered directly; the DEF_MEMBER_FUNC* helper macros are
#undef-ed after their last expansion.

Co-Authored-By: Claude Fable 5 <[email protected]>
Per-dispatch and per-variable spdlog::info calls flooded output (every
command execution logged 5+ info lines). Command/variable hot paths now
log at trace, per-component registry operations at debug; lifecycle-level
messages stay at info.

Co-Authored-By: Claude Fable 5 <[email protected]>
atom::error::Exception stream-concatenates its args, so every THROW_*
call using fmt-style placeholders produced messages like
"Precondition failed for command '"'"'{}'"'"'alwaysFail". Module-local THROW
macros now wrap fmt::format (matching THROW_REGISTRY_EXCEPTION, which
already did), and atom/error macro call sites format explicitly.

Co-Authored-By: Claude Fable 5 <[email protected]>
…acros tests

ATOM_MODULE_INIT and ATOM_HOT_COMPONENT passed 0-arg factory lambdas
where Component::InitFunc (void(Component&)) was expected, so any
ATOM_MODULE/ATOM_EMBED_MODULE expansion failed to compile, and
ATOM_VERSION was never defined. Registry gains
registerComponentInstance() to register externally created instances
(the missing API the macros needed). The disabled types_and_macros test
file targeted macros that never existed; rewritten against the real
ones (ATOM_MODULE, ATOM_EMBED_MODULE, REGISTER_INITIALIZER,
ATOM_COMPONENT). Tests: 332 -> 344.

Co-Authored-By: Claude Fable 5 <[email protected]>
The event system (Component::emitEvent/on/once/off and registry-level
subscribe/trigger) referenced atom::components::Event, EventCallback
and EventCallbackId, which were defined nowhere, so the code behind
ENABLE_EVENT_SYSTEM could never compile. Define those types in
core/types.hpp, add the ATOM_COMPONENTS_ENABLE_EVENTS CMake option
(default ON), and fix two self-deadlocks the dead code hid: cleanupAll
and initializeAll trigger events while holding mutex_, so event
subscriptions now use a dedicated eventMutex_. Tests: 344 -> 349.

Co-Authored-By: Claude Fable 5 <[email protected]>
data/type_conversion.hpp declared a TypeConverter/TypeRegistry template
API with zero definitions (link error on any use) and re-implemented
container/optional/smart-pointer/tuple traits that atom/meta already
provides. Nothing in the module used it (advanced_bindings only
included it), and its test was disabled for calling those non-existent
methods. Removed outright per the no-parallel-dead-code convention.

Co-Authored-By: Claude Fable 5 <[email protected]>
absl::StrContains was the only abseil use in the whole repository;
std::string_view::find does the same job in a constexpr context.

Co-Authored-By: Claude Fable 5 <[email protected]>
refactor: drop "advanced" qualifier and consolidate parallel files

Merge the "advanced_*" implementations into their base counterparts and
delete the redundant parallel files across the components, system, and
meta modules, following the convention that qualifier words like
"advanced" do not belong in names.

- components: advanced_bindings -> scripting/bindings
- system/command: fold advanced_executor into the executor; add command.hpp
- system/env: fold env_advanced into env_core/utils
- system/shortcut: advanced_shortcut -> shortcut_binding
- system/scheduling, system/process: drop dead duplicate sources
- meta: split refl_field.hpp out of refl; reorganize tests into
  per-area subdirectories and remove the old flat test files

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
@

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 193 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="atom/meta/constructor.hpp">

<violation number="1" location="atom/meta/constructor.hpp:547">
P2: Missing direct `#include <vector>` for newly introduced `std::vector` usage in `ObjectBuilder`</violation>
</file>

<file name="atom/meta/raw_name.hpp">

<violation number="1" location="atom/meta/raw_name.hpp:137">
P2: GCC member-name parser truncates overloaded operator member names like `operator()` due to forward search for `)` or `}` delimiters. The `find_first_of(")}", start)` call stops at the `)` inside `operator()` itself, returning `operator(` instead of the full name.</violation>
</file>

Note: This PR contains a large number of files. cubic only reviews up to 100 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
On a pro plan you can use ultrareview for larger PRs.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread atom/meta/constructor.hpp
@@ -435,30 +435,13 @@ auto asyncConstructor(std::launch policy = std::launch::async) {
template <typename Class, bool ThreadSafe = true>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Missing direct #include <vector> for newly introduced std::vector usage in ObjectBuilder

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At atom/meta/constructor.hpp, line 547:

<comment>Missing direct `#include <vector>` for newly introduced `std::vector` usage in `ObjectBuilder`</comment>

<file context>
@@ -559,40 +542,45 @@ auto factoryConstructor() {
-    std::function<std::shared_ptr<Class>()> m_buildFunc;
+    // Steps are applied in order at build(); storing them flat avoids the
+    // O(n^2) closure-chain copies of composing a new lambda per step.
+    std::vector<std::function<void(Class&)>> m_steps;
 
 public:
</file context>

Comment thread atom/meta/raw_name.hpp
if (auto start = value.rfind("::"); start != std::string_view::npos) {
start += 2;
auto end = name.rfind('}');
auto end = value.find_first_of(")}", start);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: GCC member-name parser truncates overloaded operator member names like operator() due to forward search for ) or } delimiters. The find_first_of(")}", start) call stops at the ) inside operator() itself, returning operator( instead of the full name.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At atom/meta/raw_name.hpp, line 137:

<comment>GCC member-name parser truncates overloaded operator member names like `operator()` due to forward search for `)` or `}` delimiters. The `find_first_of(")}", start)` call stops at the `)` inside `operator()` itself, returning `operator(` instead of the full name.</comment>

<file context>
@@ -108,19 +127,22 @@ constexpr std::string_view extract_enum_name(std::string_view name) noexcept {
+    if (auto start = value.rfind("::"); start != std::string_view::npos) {
         start += 2;
-        auto end = name.rfind('}');
+        auto end = value.find_first_of(")}", start);
         if (end == std::string_view::npos) {
-            end = name.size();
</file context>
Suggested change
auto end = value.find_first_of(")}", start);
auto end = value.find_last_of(")}");

Sign up for free to 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.

1 participant