Form redesign backend technology - #816
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughFORM introduces ChangesTyped technology migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@form/core/technology.hpp`:
- Around line 11-32: Rename the public technology symbols Major, Id, ROOT_TTREE,
ROOT_RNTUPLE, and HDF5 to lower_case identifiers, and move them from
form::technology into the required phlex namespace. Update every migrated call
site and related backend-dispatch usage to reference the renamed phlex symbols
while preserving their existing values and ordering behavior.
- Around line 43-44: HDF5 is currently accepted by technology parsing but
unsupported by every factory path, so prevent silent fallback. In
form/core/technology.hpp lines 43-44, reject HDF5 until complete support exists;
in form/util/factories.hpp lines 40-43, 60-64, 84-89, and 108-113, either
implement the corresponding HDF5 file, write association, read container, and
write container branches or fail explicitly instead of returning generic
storage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0b8fe1ae-e08d-45f7-a62e-fbbc62af75b5
📒 Files selected for processing (20)
form/core/placement.cppform/core/placement.hppform/core/technology.hppform/core/token.cppform/core/token.hppform/form/config.cppform/form/config.hppform/form/technology.hppform/form_module.cppform/form_source.cppform/storage/storage_reader.cppform/storage/storage_writer.cppform/util/factories.hpptest/form/form_basics_test.cpptest/form/form_root_schema_read_test.cpptest/form/form_root_schema_write_test.cpptest/form/form_storage_test.cpptest/form/reader.cpptest/form/test_utils.hpptest/form/writer.cpp
💤 Files with no reviewable changes (1)
- form/form/technology.hpp
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: build (gcc, none)
- GitHub Check: Analyze cpp with CodeQL
- GitHub Check: coverage
- GitHub Check: setup
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cc,cxx,h,hpp}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{cpp,cc,cxx,h,hpp}: Use clang-format tool for all C++ code formatting (VS Code auto-formats on save); configuration defined in.clang-formatwith 100-character line limit and 2-space indentation
Follow clang-tidy recommendations defined in.clang-tidy
Files:
test/form/form_root_schema_write_test.cpptest/form/form_root_schema_read_test.cppform/storage/storage_writer.cpptest/form/reader.cpptest/form/writer.cppform/core/placement.cppform/form_source.cppform/core/technology.hppform/core/token.hppform/core/placement.hppform/core/token.cppform/form/config.hppform/form/config.cpptest/form/form_basics_test.cppform/util/factories.hpptest/form/form_storage_test.cpptest/form/test_utils.hppform/storage/storage_reader.cppform/form_module.cpp
**/*.{hpp,cpp}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{hpp,cpp}: Use.hppfor header files,.cppfor implementation, and*_test.cppfor test files in C++
Enforce 100-character line limit and 2-space indentation in C++ code via.clang-format
UseQualifierAlignment: Right(east-const) style:int const xnotconst int xin C++
UsePointerAlignment: Leftin C++ (pointer*attached to type, not variable name)
All C++ identifiers must uselower_casenaming: namespaces, classes, structs, enums, functions, variables, parameters, members, and constants
Exception to C++ naming: template parameters useCamelCase
Exception to C++ naming: macros useUPPER_CASE
Private, protected, and constant members in C++ must have a trailing underscore (_), no trailing underscore on anything else
Useenum classpreferred over plainenumin C++
Usestd::shared_ptrfor shared ownership,std::unique_ptrfor exclusive ownership, raw pointers for non-owning references only in C++
Use functors with agent-noun pattern:ModelEvaluator evaluate_model(...)in C++
Apply.clang-tidychecks for bugprone, cert, clang-analyzer, concurrency, cppcoreguidelines, misc, modernize, performance, portability, and readability as defined in the.clang-tidyconfiguration file
Usephlex::namespace for core code,phlex::experimental::for experimental features in C++
Files:
test/form/form_root_schema_write_test.cpptest/form/form_root_schema_read_test.cppform/storage/storage_writer.cpptest/form/reader.cpptest/form/writer.cppform/core/placement.cppform/form_source.cppform/core/technology.hppform/core/token.hppform/core/placement.hppform/core/token.cppform/form/config.hppform/form/config.cpptest/form/form_basics_test.cppform/util/factories.hpptest/form/form_storage_test.cpptest/form/test_utils.hppform/storage/storage_reader.cppform/form_module.cpp
**/*.hpp
📄 CodeRabbit inference engine (AGENTS.md)
Avoid boolean parameters in C++ interfaces; prefer enumerations instead
Files:
form/core/technology.hppform/core/token.hppform/core/placement.hppform/form/config.hppform/util/factories.hpptest/form/test_utils.hpp
🪛 Cppcheck (2.21.0)
form/core/placement.cpp
[style] 22-22: The function 'technology' is never used.
(unusedFunction)
form/core/token.cpp
[style] 23-23: The function 'technology' is never used.
(unusedFunction)
form/form/config.cpp
[style] 35-35: The function 'from_string' is never used.
(unusedFunction)
[style] 34-34: The function 'getFileTable' is never used.
(unusedFunction)
[style] 41-41: The function 'getContainerTable' is never used.
(unusedFunction)
🔇 Additional comments (16)
test/form/form_basics_test.cpp (1)
1-1: LGTM!Also applies to: 29-29, 126-135, 146-146, 162-167, 180-181, 202-203, 224-224, 237-245, 301-301, 314-314, 326-326
test/form/form_root_schema_read_test.cpp (1)
17-17: LGTM!test/form/form_root_schema_write_test.cpp (1)
16-16: LGTM!test/form/form_storage_test.cpp (1)
24-24: LGTM!Also applies to: 286-287, 336-337, 372-372, 425-425, 455-456
test/form/reader.cpp (1)
3-3: LGTM!Also applies to: 39-39
test/form/test_utils.hpp (1)
42-42: LGTM!Also applies to: 51-51, 72-72, 86-86, 100-109
test/form/writer.cpp (1)
3-3: LGTM!Also applies to: 51-51
form/core/placement.hpp (1)
6-7: LGTM!Also applies to: 21-32
form/core/placement.cpp (1)
10-10: LGTM!Also applies to: 22-22
form/core/token.hpp (1)
6-7: LGTM!Also applies to: 17-34
form/core/token.cpp (1)
10-10: LGTM!Also applies to: 23-23
form/form/config.hpp (1)
4-5: LGTM!Also applies to: 17-23, 35-37, 51-56
form/form/config.cpp (1)
17-22: LGTM!Also applies to: 34-46
form/form_module.cpp (1)
7-27: LGTM!Also applies to: 105-105, 124-124
form/form_source.cpp (1)
3-3: LGTM!Also applies to: 178-178
form/core/technology.hpp (1)
26-26: 📐 Maintainability & Code QualityAdd an explicit C++20/23 flag before relying on the spaceship operator.
operator<=>requires C++20, but this workspace only showscxxstd=23in Spack configuration and no direct CMake standard setting. Add a declared-std=c++20/-std=c++23flag if the build target is not already C++20+.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #816 +/- ##
==========================================
+ Coverage 83.89% 84.17% +0.27%
==========================================
Files 171 172 +1
Lines 7234 7425 +191
Branches 850 884 +34
==========================================
+ Hits 6069 6250 +181
- Misses 881 892 +11
+ Partials 284 283 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
…zed but not yet implemented'
aolivier23
left a comment
aolivier23
left a comment
There was a problem hiding this comment.
Mostly looks good. Thank you for this improvement! There are two comments I left about things I want to discuss:
- if we want HDF5 container creation to throw for now, do we need to also protect against unknown technologies?
- Looks like we're changing the meaning of USE_RNTUPLE_STORAGE in one place but not the others. Let's discuss that please.
… into .hpp and .cpp
…lently returning the no-op generic backend; enforce RNTuple-requires-ROOT; fix form_basics_test macro visibility and add throw-path tests
aolivier23
left a comment
aolivier23
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the discussion.
aolivier23
left a comment
aolivier23
left a comment
There was a problem hiding this comment.
LGTM. I agree with Phillipe about the tests, but they're not a show-stopper for me.
aolivier23
left a comment
aolivier23
left a comment
There was a problem hiding this comment.
Thank you for the new test! LGTM.
4 participants
Summary:
Redesign of how a backend's storage technology is represented:
What changed:
technology::Idreplaces the packedint.Majoris anenum;minorselects the variant within a major (e.g., TTree vs RNTuple within ROOT).<=>gives exact (major, minor) ordering, so Id is usable directly as a std::map key and for backend dispatch.from_string/to_stringconsolidates the three duplicated parse tables that had drifted across the codebase.core/(notform/), so the core carriersPlacementandTokendon't depend upward onform/.Compatibility/Consistence:
form::technology::Id) will need a further discussion. A scan across the form/ and test/form/ folders for consistence check will be needed once we decide. This could be split into a separate PR.Core API
technology::IdwithMajor,minor, ordering, and predefined ROOT/HDF5 identifiers.from_stringandto_stringconversion.form/tocore/.FORM integration
Placement,Token, configuration, storage, and factory APIs to usetechnology::Id.Tests