revert the metadata part introduced in PR647 - #665
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:
📝 WalkthroughWalkthroughThe PR removes declared product-name and finalize plumbing from the FORM writer, persistence, and storage interfaces, updates ROOT storage container handling, and deletes related test validation code. ChangesFORM persistence and storage cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
19 fixed, 0 new since branch point (12a6178) ✅ 19 CodeQL alerts resolved since the previous PR commit
✅ 19 CodeQL alerts resolved since the branch point
Review the full CodeQL report for details. |
aolivier23
left a comment
aolivier23
left a comment
There was a problem hiding this comment.
I think we missed a function in the RNTuple interface since RNTuple compiled against main but isn't in this diff.
Otherwise looks good to me.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/form/writer.cpp (1)
10-16: 🎯 Functional Correctness | 🟡 MinorAdd
#include <string>.test/form/writer.cppusesstd::stringdirectly, and the current STL include block doesn’t cover it. Keep this test from depending on transitive includes.🤖 Prompt for 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. In `@test/form/writer.cpp` around lines 10 - 16, Add the missing standard header for std::string in the include block of writer.cpp so the test no longer relies on transitive includes; update the existing includes near the top of the file to explicitly include the string header alongside the other STL headers used by the test.form/storage/istorage.hpp (1)
56-69: 🎯 Functional Correctness | 🔴 CriticalRemove the stale
getEntryCount()override
IStorage_Write_Container,Storage_Write_Container, andStorage_Associative_Write_Containerno longer declaregetEntryCount(), soROOT_RField_Write_ContainerImp::getEntryCount() overrideinform/root_storage/root_rfield_write_container.hpp:28has no matching base virtual and will break compilation.🤖 Prompt for 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. In `@form/storage/istorage.hpp` around lines 56 - 69, The write-container interfaces no longer expose getEntryCount(), so the stale override in ROOT_RField_Write_ContainerImp is now invalid and will not compile. Remove the getEntryCount() override from ROOT_RField_Write_ContainerImp and any related implementation/declaration references, and verify the class still matches the current IStorage_Write_Container contract and the Storage_Write_Container / Storage_Associative_Write_Container hierarchy.
🤖 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.
Outside diff comments:
In `@form/storage/istorage.hpp`:
- Around line 56-69: The write-container interfaces no longer expose
getEntryCount(), so the stale override in ROOT_RField_Write_ContainerImp is now
invalid and will not compile. Remove the getEntryCount() override from
ROOT_RField_Write_ContainerImp and any related implementation/declaration
references, and verify the class still matches the current
IStorage_Write_Container contract and the Storage_Write_Container /
Storage_Associative_Write_Container hierarchy.
In `@test/form/writer.cpp`:
- Around line 10-16: Add the missing standard header for std::string in the
include block of writer.cpp so the test no longer relies on transitive includes;
update the existing includes near the top of the file to explicitly include the
string header alongside the other STL headers used by the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dfe3fcc0-5778-40e1-9cc7-5155f44d2410
📒 Files selected for processing (19)
form/form/form_writer.cppform/form/form_writer.hppform/form_module.cppform/persistence/ipersistence_writer.hppform/persistence/persistence_writer.cppform/persistence/persistence_writer.hppform/root_storage/root_tbranch_write_container.cppform/root_storage/root_tbranch_write_container.hppform/root_storage/root_tfile.cppform/root_storage/root_ttree_write_container.cppform/root_storage/root_ttree_write_container.hppform/storage/istorage.hppform/storage/storage_write_container.cppform/storage/storage_write_container.hppform/storage/storage_writer.cppform/storage/storage_writer.hpptest/form/form_storage_test.cpptest/form/form_test_with_output.jsonnettest/form/writer.cpp
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Framework-R-D/action-configure-cmake(auto-detected)Framework-R-D/action-workflow-setup(auto-detected)Framework-R-D/action-complete-pr-comment(auto-detected)Framework-R-D/action-handle-fix-commit(auto-detected)
💤 Files with no reviewable changes (10)
- form/root_storage/root_tbranch_write_container.hpp
- test/form/form_test_with_output.jsonnet
- form/storage/storage_write_container.cpp
- form/form_module.cpp
- form/root_storage/root_ttree_write_container.hpp
- form/root_storage/root_ttree_write_container.cpp
- form/root_storage/root_tbranch_write_container.cpp
- test/form/form_storage_test.cpp
- form/storage/storage_write_container.hpp
- form/form/form_writer.hpp
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Analyze cpp with CodeQL
- GitHub Check: build (gcc, none)
- GitHub Check: coverage
- GitHub Check: clang-tidy-check
🧰 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:
form/form/form_writer.cppform/root_storage/root_tfile.cppform/persistence/ipersistence_writer.hppform/storage/istorage.hppform/persistence/persistence_writer.cppform/persistence/persistence_writer.hpptest/form/writer.cppform/storage/storage_writer.hppform/storage/storage_writer.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:
form/form/form_writer.cppform/root_storage/root_tfile.cppform/persistence/ipersistence_writer.hppform/storage/istorage.hppform/persistence/persistence_writer.cppform/persistence/persistence_writer.hpptest/form/writer.cppform/storage/storage_writer.hppform/storage/storage_writer.cpp
**/*.hpp
📄 CodeRabbit inference engine (AGENTS.md)
Avoid boolean parameters in C++ interfaces; prefer enumerations instead
Files:
form/persistence/ipersistence_writer.hppform/storage/istorage.hppform/persistence/persistence_writer.hppform/storage/storage_writer.hpp
🔇 Additional comments (11)
form/persistence/ipersistence_writer.hpp (1)
30-34: LGTM!form/persistence/persistence_writer.hpp (1)
34-38: LGTM!Also applies to: 43-47
form/form/form_writer.cpp (1)
40-40: LGTM!Also applies to: 69-69
form/persistence/persistence_writer.cpp (2)
52-60: LGTM!
62-65: 🎯 Functional Correctness
indexis already special-casedfindConfigItem()maps"index"to the first configured item, socommitOutput()doesn’t need a separate metadata entry here.> Likely an incorrect or invalid review comment.test/form/writer.cpp (2)
153-154: 📐 Maintainability & Code QualityRemove this concern:
ReadVector_${TECH}already consumestoy_${TECH}.rootandtoy_checksums_${TECH}.txt, compares the checksums, and runs afterWriteVector_${TECH}viaDEPENDS.> Likely an incorrect or invalid review comment.
99-99: 🎯 Functional Correctness
std::from_rangeis fine here. The project already setsCMAKE_CXX_STANDARDto23, so no C++20 fallback is needed.> Likely an incorrect or invalid review comment.form/storage/istorage.hpp (1)
39-42: LGTM!form/storage/storage_writer.hpp (1)
22-29: LGTM!Also applies to: 31-38
form/storage/storage_writer.cpp (1)
14-17: LGTM!Also applies to: 20-69, 71-84, 87-93
form/root_storage/root_tfile.cpp (1)
11-14: 🩺 Stability & AvailabilityNo issue: the fallback matches current callers. Current code only passes
'i'and'o'intocreateFile, so theREADbranch doesn’t change existing behavior.> Likely an incorrect or invalid review comment.
|
@wwuoneway CodeRabbit has left two comments that you'll need to expand the tree to see. 
|
aolivier23
left a comment
aolivier23
left a comment
There was a problem hiding this comment.
Thank you for removing that. Looks good to me.
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #665 +/- ##
==========================================
+ Coverage 83.27% 83.55% +0.28%
==========================================
Files 162 170 +8
Lines 5912 6283 +371
Branches 670 706 +36
==========================================
+ Hits 4923 5250 +327
- Misses 796 810 +14
- Partials 193 223 +30
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 16 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Clang-Tidy Check ResultsFound 5385 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
Clang-Tidy Check ResultsFound 5372 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
5 participants
This PR reverts the changes introduced in PR #647. Additional review comments were received, and the metadata part will be added in future updates.
Code
declare_product_name,finalize, and the variousproduct_name/declared_namepaths from writer and storage interfaces/implementations.registerWrite,fillContainer, and related write/commit flows to pass only creator/label/data/type.getEntryCount()APIs; one implementation now sets tree entry counts on commit, and another exposesgetTTree()instead.ROOT_TFileImp.Tests
test/form/form_test_with_output.jsonnetfixture.test/form/writer.cppby removing the finalization call and post-write ROOT validation logic.Cleanup