Repository navigation
form: add form layout descriptor to read path - #1033
Conversation
from_string() case-folded with an arithmetic range check ('A'..'Z' and
c - 'A' + 'a'), which assumes the alphabet is contiguous. C++ guarantees
that only for the decimal digits, so replace it with two tables matched
by position.
std::tolower is still avoided on purpose: it is locale-dependent, and a
technology token is part of the on-disk layout, so the same bytes must
name the same technology whatever locale a node happens to run under. It
also has undefined behaviour for a negative char, which from_string can
see because it parses both job configuration and descriptor contents.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
| Layer / File(s) | Summary |
|---|---|
Case-insensitive technology parsing form/core/technology.hpp, test/form/form_basics_test.cpp |
from_string compares recognized names using ASCII-only case folding. Tests cover mixed-case names, rejected variations, and technology-token round-trips. |
Storage Descriptor Reads
| Layer / File(s) | Summary |
|---|---|
Optional descriptor API form/storage/istorage.hpp, form/storage/storage_file.*, form/storage/storage_reader.hpp, form/root_storage/root_tfile.hpp, test/form/form_basics_test.cpp |
Storage interfaces and storage_file expose optional descriptor values. The ROOT file and storage reader declarations add descriptor-read methods. Generic tests cover absent, populated, and empty descriptors. |
ROOT descriptor read form/root_storage/root_tfile.cpp, test/form/form_storage_test.cpp |
root_tfile_imp reads the descriptor key, returns nullopt when it is absent, and validates the object type and list entries. Tests cover absent, empty, ordered, and invalid descriptors. |
Storage reader integration form/storage/storage_reader.cpp, test/form/form_storage_test.cpp |
storage_reader centralizes file lookup, creation, settings, and caching in open_file. Descriptor reads and existing read operations use that helper. Tests cover descriptor contents, mixed ROOT backends, product reads, and file-open errors. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Feature
Sequence Diagram(s)
sequenceDiagram
participant storage_reader
participant open_file
participant root_tfile_imp
storage_reader->>open_file: Resolve or create and configure file
open_file-->>storage_reader: Return cached storage file
storage_reader->>root_tfile_imp: Read descriptor
root_tfile_imp-->>storage_reader: Return optional descriptor entries
Possibly related PRs
- Framework-R-D/phlex#1018: That PR writes the per-file descriptor that this PR reads.
Suggested reviewers: aolivier23, gemmeren
Merge Risk: 
a9e96
-
No confirmed merge-blocking issue is established from the supplied evidence. Complete normal build and test checks before merging.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title "form: add form layout descriptor to read path" matches the main change. The PR adds read_descriptor() across the storage interfaces and the ROOT implementation, and it makes technology pars… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "form: add form layout descriptor to read path" matches the main change. The PR adds read_descriptor() across the storage interfaces and the ROOT implementation, and it makes technology parsing case-insensitive. The title names the core feature, which is the descriptor read path. It does not name the case-insensitive technology parsing, but the changeset does not center on that, so this omission is acceptable.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 7 functions across 10 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1033 +/- ##
==========================================
- Coverage 89.65% 89.57% -0.09%
==========================================
Files 177 177
Lines 5996 6006 +10
Branches 994 996 +2
==========================================
+ Hits 5376 5380 +4
- Misses 392 398 +6
Partials 228 228
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
@coderabbitai Please open an issue to track separating the configuration spelling from the on-disk technology token. The current technology_name() derives the on-disk token by lowercasing the configuration name. Ideally, these should be declared independently, with the descriptor parser using an exact match rather than case folding. This is a follow-up design improvement and should be handled in a separate PR, not included in the current one. |
|
Tip For best results, initiate chat on the files or code changes. Analysis chain
The issue tracks independent configuration spellings and on-disk technology tokens, exact-match descriptor parsing, and regression tests. It links back to your request and specifies a separate PR from You are interacting with an AI system. |
gemmeren
left a comment
gemmeren
left a comment
There was a problem hiding this comment.
one small comment for you to consider.
|
@coderabbitai please update #1034 to match what landed in #1033. The case folding this issue was written against is no longer in the codebase. It was dropped during review of #1033: What should change here:
Everything else stands: independent declarations, exact-match descriptor parsing, keeping configuration parsing separate, and preserving the existing on-disk tokens. |
|
Tip For best results, initiate chat on the files or code changes. Analysis chain
The issue still requires independent declarations, separate configuration parsing, preserved on-disk tokens, and implementation in a separate PR. Learnings added — pending approval
You are interacting with an AI system. |
aolivier23
left a comment
aolivier23
left a comment
There was a problem hiding this comment.
LGTM. Just make sure tests are passing please. One of them looked a little suspicious to me on first glance, and I left a comment.
A technology's on-disk token was derived by lowercasing its configuration spelling, tying a persistent file-format identifier to a user-interface string: renaming a configuration name would silently have changed what FORM writes to disk. There was no parser in the other direction either, so the descriptor read back in #1033 could not be turned into a technology::id. Declare both spellings in one constexpr registry in core/technology.hpp, holding each technology's id, configuration name, on-disk token and whether FORM implements it. Every lookup and both parsers derive from that table. Add a separate exact-match parsing path for on-disk tokens: on_disk_token(), find_on_disk_token() and from_on_disk_token().
Summary
The write path gained a layout descriptor in #1018 , but nothing can read it —
i_storage_fileonly declareswrite_descriptor. This adds the read side, so a reader can ask a file which layout version and which technology it holds.It adds
read_descriptor()to the storage layer, along with the ROOT implementation.Nothing consumes the descriptor yet; this is the prerequisite step for the navigation-aware read path.
read_descriptor()to the storage reader and file interfaces. It returns an optional descriptor, distinguishing a missing descriptor from an empty one.TNamednames and titles in order, and reports invalid descriptor objects or entries.storage_reader. Descriptor reads and existing read operations use this helper.