Skip to content

Refactor forest domain protocol - #338

Merged
netkeep80 merged 4 commits into
netkeep80:mainfrom
konard:issue-335-e5e62876c0ee
Apr 20, 2026
Merged

Refactor forest domain protocol#338
netkeep80 merged 4 commits into
netkeep80:mainfrom
konard:issue-335-e5e62876c0ee

Conversation

@konard

@konard konard commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Make the forest-domain contract explicit with a read-only ForestDomainViewDescriptor/ForestDomainViewOps surface and a mutable ForestDomainDescriptor/ForestDomainOps surface.
  • Keep find, root_index, and name available from const contexts while restricting root_index_ptr, insert, and reset_root to mutable handles.
  • Remove pmap::forest_domain_ops() const and the _root_idx const_cast; const pmap now exposes only forest_domain_view_ops() plus normal read APIs (find, contains).
  • Move pmap onto the same forest-domain protocol as pstringview, including keyed lookup through compare_key(key, node_pptr).
  • Route pstringview/system/symbols, symbol bootstrap interning, public legacy root access, and public domain-root access through the canonical forest-domain root-index helpers.
  • Refresh generated single-header outputs and update the changelog fragment.

Fixes #335

Removed duplicate paths

  • Removed pmap::_avl_find() and pmap::_avl_insert() as local wrappers around raw AVL helpers.
  • Removed pstringview::_avl_find() and pstringview::_avl_insert() as local wrappers around ForestDomainOps.
  • Removed get_legacy_root_offset_unlocked(), set_legacy_root_offset_unlocked(), and symbol_domain_root_offset_unlocked() in favor of forest_domain_root_index_unlocked(), forest_domain_root_index_ptr_unlocked(), and set_forest_domain_root_index_unlocked().
  • Removed comments that described the old _avl_find() helper path as canonical test intent.

Canonical protocol

Read-only descriptor/view core:

  • name()
  • root_index()
  • resolve_node(node_pptr)
  • optional compare_key(key, node_pptr) for keyed lookup

Mutable descriptor/ops core:

  • root_index_ptr()
  • less_node(lhs, rhs)
  • optional validate_node(node_pptr)
  • insert(node)
  • reset_root()

Const-correctness

  • ForestDomainOps::insert, ForestDomainOps::reset_root, and ForestDomainOps::root_index_ptr are no longer const methods.
  • pmap::forest_domain_descriptor keeps separate read and mutable root slots; descriptors created from const pmap have no mutable root slot.
  • tests/test_issue153_pmap.cpp statically asserts that const pmap cannot call forest_domain_ops() and that const policy handles cannot call insert, reset_root, or root_index_ptr.

Line-count impact

Hand-written core include files changed by net +56 lines:

  • include/pmm/avl_tree_mixin.h: +40
  • include/pmm/forest_domain_mixin.inc: -3
  • include/pmm/persist_memory_manager.h: 0
  • include/pmm/pmap.h: +23
  • include/pmm/pstringview.h: -4

The additional lines are mostly the explicit read-only/mutable split requested in PR feedback. The duplicate helper paths listed above remain removed.

Reproduction

Added forest-domain descriptor drives pmap dictionary in tests/test_issue153_pmap.cpp.

Before the original implementation, cmake --build build --target test_issue153_pmap failed to compile because Map::forest_domain_descriptor and map.forest_domain_ops() did not exist.

Before the const-correctness follow-up, the added static assertions failed because const pmap exposed forest_domain_ops() and const ForestDomainOps still allowed insert, reset_root, and root_index_ptr.

Verification

  • cmake --build build --target test_issue153_pmap test_issue151_pstringview test_issue162_deduplication test_forest_registry
  • ./build/tests/test_issue153_pmap (268 assertions, 16 test cases)
  • ./build/tests/test_issue151_pstringview (439 assertions, 26 test cases)
  • ./build/tests/test_issue162_deduplication (117 assertions, 11 test cases)
  • ./build/tests/test_forest_registry (75 assertions, 3 test cases)
  • cmake --build build
  • ctest --test-dir build --output-on-failure (92/92 passed)
  • clang-format --dry-run --Werror on touched source/test files
  • bash scripts/check-file-size.sh
  • GITHUB_BASE_REF=main bash scripts/check-changelog-fragment.sh
  • bash scripts/check-docs-consistency.sh
  • bash scripts/check-repo-guard-rollout.sh
  • git diff --check
  • bash scripts/generate-single-headers.sh --strip-comments --output-dir /tmp/generated-pmm-issue335-final plus diff -q against committed generated headers

Local notes

  • cppcheck was not available in this runner (cppcheck: command not found), so local cppcheck was not run.
  • Direct local bash scripts/check-version-consistency.sh reports the existing release-owned README badge mismatch (0.55.12 vs 0.57.7). The GitHub workflow skips that check for this PR because no release-owned paths changed.

konard added 2 commits April 20, 2026 17:17
Adding .gitkeep for PR creation (default mode).
This file will be removed when the task is complete.

Issue: netkeep80#335
@konard konard changed the title [WIP] # Issue 1. Ввести минимальный forest domain protocol и удалить дублирующие пути Refactor forest domain protocol Apr 20, 2026
@konard
konard marked this pull request as ready for review April 20, 2026 17:43
@konard

konard commented Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

Solution summary

Implemented and pushed the fix for issue 335.

PR 338 is ready for review: #338
Commit: f9daddaba4042e185dc4a91131754fab7710c725

What changed:

  • Made ForestDomainOps<Domain> the canonical value-based forest-domain protocol wrapper.
  • Added ForestDomainDescriptor plus keyed ForestDomainDescriptorForKey.
  • Moved pmap onto the same protocol via pmap::forest_domain_descriptor and forest_domain_ops().
  • Removed duplicate _avl_find/_avl_insert wrapper paths from pmap and pstringview.
  • Collapsed legacy/symbol root helpers into canonical root-index helpers.
  • Regenerated single_include/pmm/pmm.h and pmm_no_comments.h.
  • Added a changelog fragment and updated PR title/body with removed paths, canonical protocol, and line-count impact.

Verification:

  • Local cmake --build build passed.
  • Local ctest --test-dir build --output-on-failure passed: 92/92.
  • Targeted tests passed:
    • test_issue153_pmap
    • test_issue151_pstringview
    • test_issue162_deduplication
    • test_forest_registry
  • Local checks passed: touched-file clang-format, file-size, changelog fragment, single-header freshness, git diff --check, repo-guard rollout.
  • GitHub CI is green on the pushed commit, including cppcheck, matrix builds, sanitizers, demos, docs consistency, and single-header freshness: https://github.com/netkeep80/PersistMemoryManager/actions/runs/24681348894

Working tree is clean.

Next useful steps:

  1. Review PR 338’s diff, especially pmap::forest_domain_descriptor and the root-index helper consolidation.
  2. Merge PR 338 after human review.

This summary was automatically extracted from the AI working session output.

@konard

konard commented Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Solution Draft Log

This log file contains the complete execution trace of the AI solution draft process.

💰 Cost estimation:

  • Model: GPT-5.4
  • Provider: OpenAI
  • Public pricing estimate: $16.969117

📊 Context and tokens usage:

  • 29.6M / 1.1M (2817%) input tokens, 47.8K / 128K (37%) output tokens

Total: (246.1K + 29.3M cached) input tokens, 47.8K output tokens, $16.969117 cost

🤖 Models used:

  • Tool: OpenAI Codex
  • Requested: gpt-5.4
  • Model: GPT-5.4 (gpt-5.4)

📎 Log file uploaded as Repository (55290KB)


Now working session is ended, feel free to review and add any feedback on the solution draft.

@konard

konard commented Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

✅ Ready to merge

This pull request is now ready to be merged:

  • All CI checks have passed
  • No merge conflicts
  • No pending changes

Monitored by hive-mind with --auto-restart-until-mergeable flag

@netkeep80

Copy link
Copy Markdown
Owner

Вот комментарий, который я бы оставил под PR:

В целом направление правильное: ForestDomainOps<Domain> действительно становится заметно более каноническим слоем, а удаление локальных _avl_find/_avl_insert из pmap и pstringview — хороший шаг к упрощению.

Но я бы всё-таки попросил исправить одну важную вещь перед merge:

pmap::forest_domain_ops() const сейчас возвращает mutable protocol handle через const_cast( &_root_idx ), а сами mutating-методы ForestDomainOps (insert, reset_root) объявлены как const. В итоге через const pmap можно получить handle, который меняет _root_idx. Это ломает const-correctness и делает protocol слишком permissive.

Мне кажется, здесь лучше выбрать один из двух вариантов:

  1. либо убрать forest_domain_ops() const совсем и оставить только read-only доступ через отдельные методы;
  2. либо разделить protocol на read-only и mutable surface, чтобы find/root_index/name были доступны из const-контекста, а insert/reset_root/root_index_ptr — только из mutable.

Иначе получается, что новый “canonical protocol” уже чище старого пути, но всё ещё смешивает чтение и запись в одном объекте и делает это ценой const_cast. Для слоя, который должен стать базовым forest-domain contract, это слишком слабая граница.

@konard
konard marked this pull request as draft April 20, 2026 17:54
@konard

konard commented Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

🤖 AI Work Session Started

Starting automated work session at 2026-04-20T17:54:47.420Z

The PR has been converted to draft mode while work is in progress.

This comment marks the beginning of an AI work session. Please wait for the session to finish, and provide your feedback.

@konard
konard marked this pull request as ready for review April 20, 2026 18:14
@konard

konard commented Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

Updated the PR for the const-correctness review feedback and marked it ready for review.

What changed:

  • Split the forest-domain protocol into read-only ForestDomainViewOps / ForestDomainViewDescriptor and mutable ForestDomainOps / ForestDomainDescriptor surfaces.
  • Removed the const pmap::forest_domain_ops() mutating path and the const_cast around the root index slot.
  • Added regression static assertions proving const pmap exposes only read-only domain operations.
  • Regenerated single-header outputs.

Verification:

Note: a direct local scripts/check-version-consistency.sh still reports the existing README badge/CMake version mismatch, but the release-owned CI version check passed because this PR does not touch release-owned files.

@konard

konard commented Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Solution Draft Log

This log file contains the complete execution trace of the AI solution draft process.

💰 Cost estimation:

  • Model: GPT-5.4
  • Provider: OpenAI
  • Public pricing estimate: $9.965145

📊 Context and tokens usage:

  • 15.5M / 1.1M (1478%) input tokens, 32.7K / 128K (26%) output tokens

Total: (326.3K + 15.2M cached) input tokens, 32.7K output tokens, $9.965145 cost

🤖 Models used:

  • Tool: OpenAI Codex
  • Requested: gpt-5.4
  • Model: GPT-5.4 (gpt-5.4)

📎 Log file uploaded as Repository (38584KB)


Now working session is ended, feel free to review and add any feedback on the solution draft.

@konard

konard commented Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

✅ Ready to merge

This pull request is now ready to be merged:

  • All CI checks have passed
  • No merge conflicts
  • No pending changes

Monitored by hive-mind with --auto-restart-until-mergeable flag

Copy link
Copy Markdown
Owner

В целом направление правильное: ForestDomainOps<Domain> действительно становится заметно более каноническим слоем, а удаление локальных _avl_find/_avl_insert из pmap и pstringview — хороший шаг к упрощению.

Но я бы всё-таки попросил исправить одну важную вещь перед merge:

pmap::forest_domain_ops() const сейчас возвращает mutable protocol handle через const_cast(&_root_idx), а сами mutating-методы ForestDomainOps (insert, reset_root) объявлены как const. В итоге через const pmap можно получить handle, который меняет _root_idx.

Это ломает const-correctness и делает protocol слишком permissive.

Мне кажется, здесь лучше выбрать один из двух вариантов:

  1. либо убрать forest_domain_ops() const совсем и оставить только read-only доступ через отдельные методы;
  2. либо разделить protocol на read-only и mutable surface, чтобы find/root_index/name были доступны из const-контекста, а insert/reset_root/root_index_ptr — только из mutable.

Иначе получается, что новый “canonical protocol” уже чище старого пути, но всё ещё смешивает чтение и запись в одном объекте и делает это ценой const_cast. Для слоя, который должен стать базовым forest-domain contract, это слишком слабая граница.

@konard

konard commented Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

🤖 AI Work Session Started

Starting automated work session at 2026-04-20T18:19:59.117Z

The PR has been converted to draft mode while work is in progress.

This comment marks the beginning of an AI work session. Please wait for the session to finish, and provide your feedback.

@konard
konard marked this pull request as draft April 20, 2026 18:20
@konard
konard marked this pull request as ready for review April 20, 2026 18:30
@konard

konard commented Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

Verified the latest repeated const-correctness feedback against branch head 6a59aa341b9bef290ecc703bc517ae98fae95758.

No additional code changes were needed: the current branch already removes pmap::forest_domain_ops() const, keeps const pmap on forest_domain_view_ops(), and makes ForestDomainOps::insert, reset_root, and root_index_ptr non-const. tests/test_issue153_pmap.cpp includes static assertions for those boundaries.

Local verification rerun in this session:

  • cmake -B build -DCMAKE_BUILD_TYPE=Debug
  • cmake --build build --target test_issue153_pmap test_issue151_pstringview test_issue162_deduplication test_forest_registry
  • ctest --test-dir build --output-on-failure -R 'test_issue153_pmap|test_issue151_pstringview|test_issue162_deduplication|test_forest_registry' (4/4 passed)
  • cmake --build build
  • ctest --test-dir build --output-on-failure (92/92 passed)
  • touched-file clang-format --dry-run --Werror
  • bash scripts/check-file-size.sh
  • GITHUB_BASE_REF=main bash scripts/check-changelog-fragment.sh
  • bash scripts/check-docs-consistency.sh
  • bash scripts/check-repo-guard-rollout.sh
  • git diff --check
  • regenerated single headers into /tmp/generated-pmm-issue335-verify and diff -qr matched single_include/pmm

Notes:

  • Local cppcheck is unavailable in this runner (cppcheck: command not found), but GitHub cppcheck is green for the same head SHA.
  • Direct local scripts/check-version-consistency.sh still reports the existing release-owned README badge mismatch (0.55.12 vs 0.57.7); the PR does not touch release-owned paths and the GitHub release-owned check is green.

PR is marked ready for review again.

@konard

konard commented Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Solution Draft Log

This log file contains the complete execution trace of the AI solution draft process.

💰 Cost estimation:

  • Model: GPT-5.4
  • Provider: OpenAI
  • Public pricing estimate: $3.266316

📊 Context and tokens usage:

  • 5.0M / 1.1M (476%) input tokens, 16.5K / 128K (13%) output tokens

Total: (87.6K + 4.9M cached) input tokens, 16.5K output tokens, $3.266316 cost

🤖 Models used:

  • Tool: OpenAI Codex
  • Requested: gpt-5.4
  • Model: GPT-5.4 (gpt-5.4)

📎 Log file uploaded as Gist (20269KB)


Now working session is ended, feel free to review and add any feedback on the solution draft.

@konard

konard commented Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

✅ Ready to merge

This pull request is now ready to be merged:

  • All CI checks have passed
  • No merge conflicts
  • No pending changes

Monitored by hive-mind with --auto-restart-until-mergeable flag

@netkeep80
netkeep80 merged commit a2f9e3a into netkeep80:main Apr 20, 2026
20 checks passed
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.

# Issue 1. Ввести минимальный forest domain protocol и удалить дублирующие пути

2 participants