FORM read files without metadata#674
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 extends FORM and storage reader interfaces with priming and index enumeration, adds ROOT-backed implementations and structured index parsing, replaces FORM provider registration with a registry-backed source, and adds Gaussian-based test modules and Jsonnet pipelines. ChangesFORM reader, storage, and source pipeline
Sequence Diagram(s)sequenceDiagram
participant FormInputSource
participant form_source_type_registry
participant form_reader_interface
participant PersistenceReader
participant StorageReader
FormInputSource->>form_source_type_registry: find_form_product_type(product_type)
form_source_type_registry-->>FormInputSource: form_source_type_entry
FormInputSource->>form_reader_interface: prime(actual_creator, product_name, type)
form_reader_interface->>PersistenceReader: prime(...)
PersistenceReader->>StorageReader: prime(...)
FormInputSource->>form_reader_interface: indices(actual_creator, product_name)
form_reader_interface->>PersistenceReader: listIndices(...)
PersistenceReader->>StorageReader: listIndices(...)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ 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 |
|
21 fixed, 0 new since branch point (7409297) ✅ 21 CodeQL alerts resolved since the previous PR commit
✅ 21 CodeQL alerts resolved since the branch point
Review the full CodeQL report for details. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 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/form_source.cpp`:
- Around line 158-167: The creator resolution logic in form_source.cpp is too
broad because the catch-all around config.get for algorithm and plugin hides
real failures and silently falls back to advertised_creator. Update the
actual_creator assignment to use the existing default-based config.get<T>(key,
default) overload, similar to the technology/module_label pattern, so missing
keys are treated as optional without swallowing unexpected exceptions. Keep the
actual_creator/plugin:algorithm composition in place, but remove the catch (...)
block so malformed values and other real errors remain visible.
- Around line 116-125: The current `indices()` implementation in
`form_source.cpp` only passes `products_.front()` into `reader_->indices()`, but
the comment indicates the backend in `persistence_reader.cpp` ignores that
argument and always looks up the hard-coded `"index"` key. Confirm the intended
model for `reader_->indices(actual_creator_, ...)` and `persistence_reader`—if
product-specific indices are required, wire `product_name` into the storage
lookup key in `persistence_reader.cpp`; otherwise keep the current single shared
index behavior and update the signature/comments to reflect that `product_name`
is intentionally unused.
- Around line 40-46: The FORM index parsing in the component handling logic
needs explicit validation before calling std::stoull so malformed input does not
surface as a cryptic exception or allow an empty layer. Update the parsing in
the code path that extracts layer and number from trimmed to check that the
colon is neither missing nor at position 0, verify the number substring is
non-empty and numeric, and then raise a std::runtime_error with a clear message
when the input is invalid. Keep the behavior consistent with the existing
parser’s friendly error handling around FORM index components.
In `@form/form/form_source_type_registry.cpp`:
- Around line 3-6: form_source_type_registry.cpp is relying on transitive
includes for standard library symbols it uses. Add the missing direct standard
headers for std::type_info/typeid as well as the std::string and stdexcept
dependencies to form_source_type_registry.cpp, keeping the existing
form_source_type_registry.hpp usage intact and making the implementation file
self-sufficient.
- Around line 42-67: The lookup helpers in form_source_type_registry.cpp return
pointers into the registry after releasing the mutex, so they are only safe if
registration is complete before any lookup. Update find_form_product_type and
find_form_product_type_name to make this contract explicit by documenting the
init-time-only assumption near
register_form_product_type/ensure_builtin_form_product_types_registered, or
enforce it with an internal guard that prevents late registration after lookups
begin. Keep the fix focused on the registry APIs and their pointer-returning
behavior.
In `@form/persistence/persistence_reader.cpp`:
- Line 77: Remove the no-op cast for label in the relevant persistence reader
function, since label is actively used in the lookup and error handling. Update
the function that calls findConfigItem so the leftover (void)label; line is
deleted, leaving the existing label references intact and making the code
accurately reflect its usage.
In `@form/root_storage/root_rfield_read_container.cpp`:
- Around line 43-56: The ROOT_RField_Read_ContainerImp lazy initialization in
prime() and read() is not thread-safe, unlike ROOT_TBranch_Read_ContainerImp.
Add a static mutex for this container implementation and guard the m_reader
setup in prime() plus the m_view initialization/use in read() with
std::lock_guard so concurrent StorageReader access cannot race on these members.
In `@form/root_storage/root_tbranch_read_container.cpp`:
- Around line 43-78: Extract the repeated file-to-tree-to-branch lazy-attach and
validation logic from ROOT_TBranch_Read_ContainerImp::prime, read, and entries
into a private helper such as ensure_tree_and_branch() that is called only after
the mutex is already held. Have the helper centralize the null checks and lazy
Get<TTree>()/GetBranch() lookup so all three methods share the same behavior and
error strings, avoiding the current drift between ROOT_TBranch_Read_ContainerImp
and ROOT_TBranch_ContainerImp messages.
In `@form/storage/storage_reader.cpp`:
- Around line 17-48: The lookup helpers in storage_reader.cpp are duplicated
from storage_writer.cpp, so extract the shared pure lookup logic from
get_file_table and get_container_table into a common location used by both
reader and writer. Move these functions out of the anonymous namespace into a
shared header or utility near tech_setting_config, then update both
storage_reader.cpp and storage_writer.cpp to include and call the shared helpers
so the fallback behavior stays consistent in one place.
- Around line 261-266: The all-zero normalized query branch in getIndex is
returning a sentinel 0 even when the empty-string key is found, which diverges
from the normal lookup path that returns the mapped entry value. Update the
storage_reader.cpp logic in getIndex so that, after finding empty_key in
m_indexMaps[token.containerName()], it returns empty_key->second instead of 0,
and add a short comment only if 0 is meant to be a special contract.
- Around line 65-86: parse_index_number is inferring base 16 from any alphabetic
digit, which can collapse distinct inputs like a and 10 into the same numeric
value. Update parse_index_number in storage_reader.cpp to use deterministic
parsing: either parse all unprefixed values strictly as base 10, or only switch
to base 16 when an explicit prefix like 0x is present. Keep the existing
error/nullopt behavior, and ensure callers of parse_index_number no longer rely
on implicit hex detection.
In `@test/form/form_gaussian_read_test.jsonnet`:
- Line 16: The Jsonnet fixture comment style is inconsistent with the formatter
expected by CI, and the `advertised_creator` placeholder comments in the form
Gaussian read test will fail `jsonnetfmt --test`. Update the commented
placeholder lines in `form_gaussian_read_test.jsonnet` to use hash-style
comments like the matching `form_gaussian_write_test.jsonnet`, or remove the
placeholders entirely if they are not needed, so the fixture stays
formatter-compliant.
In `@test/form/form_gaussian_write_test.jsonnet`:
- Line 1: Fix the typo in the comment on the wire_size declaration by changing
“afew” to “a few” in form_gaussian_write_test.jsonnet. Keep the existing #
comment style as-is; the only needed change is the wording in that comment.
In `@test/form/generate_vector.cpp`:
- Around line 34-35: The seed default in generate_vector uses
time_since_epoch().count() as the fallback for config.get<int>, which narrows a
64-bit value into int and can trigger conversion warnings. Update the seed
retrieval in generate_vector so the default value is explicitly converted to int
before passing it to config.get, making the truncation intentional and clear.
- Around line 1-7: The test file relies on transitive includes for standard
library symbols, so make its dependencies explicit. Add the missing standard
headers needed by the uses in the test, especially the header for assert and any
headers required for std::vector and size_t, and keep the existing phlex
includes unchanged. Check the includes at the top of generate_vector.cpp and
ensure every directly used type or macro is covered by its own include.
🪄 Autofix (Beta)
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: 4b2db1c2-bdba-494f-9537-cdfa8d01893d
📒 Files selected for processing (24)
form/form/CMakeLists.txtform/form/form_reader.cppform/form/form_reader.hppform/form/form_source_type_registry.cppform/form/form_source_type_registry.hppform/form_source.cppform/persistence/ipersistence_reader.hppform/persistence/persistence_reader.cppform/persistence/persistence_reader.hppform/root_storage/root_rfield_read_container.cppform/root_storage/root_rfield_read_container.hppform/root_storage/root_tbranch_read_container.cppform/root_storage/root_tbranch_read_container.hppform/storage/istorage.hppform/storage/storage_read_container.cppform/storage/storage_read_container.hppform/storage/storage_reader.cppform/storage/storage_reader.hppform/storage/storage_writer.cpptest/form/CMakeLists.txttest/form/form_gaussian_read_test.jsonnettest/form/form_gaussian_write_test.jsonnettest/form/form_source_extra_types.cpptest/form/generate_vector.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Analyze cpp with CodeQL
- GitHub Check: build (gcc, none)
- GitHub Check: clang-tidy-check
- GitHub Check: coverage
⚠️ CI failures not shown inline (7)
GitHub Actions: wwuoneway checking C++ code format / 0_clang-format-check.txt: wwuoneway checking C++ code format
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "failure" = 'success' ]; then�[0m
�[36;1m echo "✅ clang-format check passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::clang-format check failed. Please review the output above for details."�[0m
GitHub Actions: wwuoneway checking Jsonnet format / jsonnet-format-check: wwuoneway checking Jsonnet format
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "failure" = 'success' ]; then�[0m
�[36;1m echo "✅ Jsonnet formatting check passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::Jsonnet formatting issues found. Please review the output above for details."�[0m
GitHub Actions: wwuoneway checking C++ code format / clang-format-check: wwuoneway checking C++ code format
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "failure" = 'success' ]; then�[0m
�[36;1m echo "✅ clang-format check passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::clang-format check failed. Please review the output above for details."�[0m
GitHub Actions: wwuoneway checking Jsonnet format / jsonnet-format-check: wwuoneway checking Jsonnet format
Conclusion: failure
##[group]Run find . \( -name "*.jsonnet" -o -name "*.libsonnet" \) -print0 | xargs -0 -r -I {} \
�[36;1mfind . \( -name "*.jsonnet" -o -name "*.libsonnet" \) -print0 | xargs -0 -r -I {} \�[0m
�[36;1m bash -c 'jsonnetfmt --test "{}" || (echo "FAILED: {}" && diff -u <(jsonnetfmt "{}") "{}" && exit 1)'�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
./test/form/form_gaussian_write_test.jsonnet
FAILED: ./test/form/form_gaussian_write_test.jsonnet
--- /dev/fd/63 .283134194 +0000
+++ ./test/form/form_gaussian_write_test.jsonnet .402806230 +0000
@@ -1,4 +1,4 @@
-local wire_size = 1024; // ~600MB file in afew seconds with 4 threads on a GPVM
+local wire_size = 1024; # ~600MB file in afew seconds with 4 threads on a GPVM
{
driver: {
##[error]Process completed with exit code 123.
GitHub Actions: wwuoneway checking Jsonnet format / 0_jsonnet-format-check.txt: wwuoneway checking Jsonnet format
Conclusion: failure
##[group]Run find . \( -name "*.jsonnet" -o -name "*.libsonnet" \) -print0 | xargs -0 -r -I {} \
�[36;1mfind . \( -name "*.jsonnet" -o -name "*.libsonnet" \) -print0 | xargs -0 -r -I {} \�[0m
�[36;1m bash -c 'jsonnetfmt --test "{}" || (echo "FAILED: {}" && diff -u <(jsonnetfmt "{}") "{}" && exit 1)'�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
./test/form/form_gaussian_write_test.jsonnet
FAILED: ./test/form/form_gaussian_write_test.jsonnet
--- /dev/fd/63 .283134194 +0000
+++ ./test/form/form_gaussian_write_test.jsonnet .402806230 +0000
@@ -1,4 +1,4 @@
-local wire_size = 1024; // ~600MB file in afew seconds with 4 threads on a GPVM
+local wire_size = 1024; # ~600MB file in afew seconds with 4 threads on a GPVM
{
driver: {
##[error]Process completed with exit code 123.
GitHub Actions: wwuoneway checking CMake format / cmake-format-check: wwuoneway checking CMake format
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "success" = 'success' ]; then�[0m
�[36;1m echo "✅ All CMake files are properly formatted."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::Found files with formatting issues."�[0m
GitHub Actions: wwuoneway checking CMake format / 0_cmake-format-check.txt: wwuoneway checking CMake format
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "success" = 'success' ]; then�[0m
�[36;1m echo "✅ All CMake files are properly formatted."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::Found files with formatting issues."�[0m
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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_source_extra_types.cppform/form/form_reader.cppform/root_storage/root_tbranch_read_container.hppform/form/form_reader.hppform/storage/storage_reader.hppform/storage/storage_read_container.cppform/root_storage/root_rfield_read_container.cppform/root_storage/root_rfield_read_container.hppform/persistence/persistence_reader.hpptest/form/generate_vector.cppform/form/form_source_type_registry.hppform/persistence/persistence_reader.cppform/persistence/ipersistence_reader.hppform/storage/istorage.hppform/form/form_source_type_registry.cppform/storage/storage_writer.cppform/storage/storage_read_container.hppform/root_storage/root_tbranch_read_container.cppform/storage/storage_reader.cppform/form_source.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_source_extra_types.cppform/form/form_reader.cppform/root_storage/root_tbranch_read_container.hppform/form/form_reader.hppform/storage/storage_reader.hppform/storage/storage_read_container.cppform/root_storage/root_rfield_read_container.cppform/root_storage/root_rfield_read_container.hppform/persistence/persistence_reader.hpptest/form/generate_vector.cppform/form/form_source_type_registry.hppform/persistence/persistence_reader.cppform/persistence/ipersistence_reader.hppform/storage/istorage.hppform/form/form_source_type_registry.cppform/storage/storage_writer.cppform/storage/storage_read_container.hppform/root_storage/root_tbranch_read_container.cppform/storage/storage_reader.cppform/form_source.cpp
**/*.jsonnet
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
jsonnetfmtfor consistent Jsonnet formatting; CI enforces thisUse
jsonnetfmtformatting for Jsonnet files; CI enforces this
Files:
test/form/form_gaussian_write_test.jsonnettest/form/form_gaussian_read_test.jsonnet
**/*.hpp
📄 CodeRabbit inference engine (AGENTS.md)
Avoid boolean parameters in C++ interfaces; prefer enumerations instead
Files:
form/root_storage/root_tbranch_read_container.hppform/form/form_reader.hppform/storage/storage_reader.hppform/root_storage/root_rfield_read_container.hppform/persistence/persistence_reader.hppform/form/form_source_type_registry.hppform/persistence/ipersistence_reader.hppform/storage/istorage.hppform/storage/storage_read_container.hpp
🪛 Cppcheck (2.21.0)
form/form/form_reader.cpp
[style] 45-45: The function 'indices' is never used.
(unusedFunction)
form/storage/storage_read_container.cpp
[style] 38-38: The function 'getItems' is never used.
(unusedFunction)
form/root_storage/root_rfield_read_container.cpp
[style] 43-43: The function 'prime' is never used.
(unusedFunction)
[style] 112-112: The function 'entries' is never used.
(unusedFunction)
form/form/form_source_type_registry.cpp
[style] 38-38: The function 'getItems' is never used.
(unusedFunction)
[style] 42-42: The function 'find_form_product_type' is never used.
(unusedFunction)
[style] 55-55: The function 'find_form_product_type_name' is never used.
(unusedFunction)
form/storage/storage_writer.cpp
[style] 38-38: The function 'getItems' is never used.
(unusedFunction)
form/root_storage/root_tbranch_read_container.cpp
[style] 43-43: The function 'prime' is never used.
(unusedFunction)
[style] 80-80: The function 'read' is never used.
(unusedFunction)
[style] 191-191: The function 'entries' is never used.
(unusedFunction)
form/storage/storage_reader.cpp
[style] 38-38: The function 'getItems' is never used.
(unusedFunction)
[style] 45-45: The function 'createWriteAssociation' is never used.
(unusedFunction)
[style] 94-94: The function 'createWriteContainer' is never used.
(unusedFunction)
form/form_source.cpp
[style] 38-38: The function 'getItems' is never used.
(unusedFunction)
[style] 69-69: The function 'register_form_vector_product_type' is never used.
(unusedFunction)
[style] 21-21: The function 'GetMajor' is never used.
(unusedFunction)
[style] 22-22: The function 'GetMinor' is never used.
(unusedFunction)
🪛 GitHub Actions: wwuoneway checking Jsonnet format / 0_jsonnet-format-check.txt
test/form/form_gaussian_write_test.jsonnet
[error] 1-4: jsonnetfmt --test failed (formatting diff detected). FAILED: ./test/form/form_gaussian_write_test.jsonnet. Diff shows comment style change required: local wire_size = 1024; // ~600MB file... -> local wire_size = 1024; # ~600MB file....
🪛 GitHub Actions: wwuoneway checking Jsonnet format / jsonnet-format-check
test/form/form_gaussian_write_test.jsonnet
[error] 1-1: jsonnetfmt --test failed (format mismatch). FAILED diff: comment style changed from '// ~600MB file in afew seconds with 4 threads on a GPVM' to '# ~600MB file in afew seconds with 4 threads on a GPVM'. Step command: jsonnetfmt --test "{}" || (echo "FAILED: {}" && diff -u <(jsonnetfmt "{}") "{}" && exit 1)
🔇 Additional comments (20)
form/form/CMakeLists.txt (1)
6-10: LGTM!form/form/form_source_type_registry.hpp (1)
17-72: LGTM!test/form/CMakeLists.txt (1)
159-164: LGTM!test/form/form_source_extra_types.cpp (1)
6-31: LGTM!form/form/form_reader.hpp (1)
28-32: LGTM!form/persistence/ipersistence_reader.hpp (1)
10-10: LGTM!Also applies to: 34-40
form/storage/istorage.hpp (2)
86-86: 🎯 Functional Correctness | 💤 Low value
entries()returningintwill silently truncate on big trees.
TTree::GetEntries()/RNTupleReader::GetNEntries()hand backLong64_t/ULong64_t. HEP files routinely blow past 2.1B entries, and a narrowing cast tointwraps negative without a peep. I get thatread(int id, ...)already pins the whole indexing scheme toint, so this is really a pre-existing design ceiling rather than a new bug — just flagging so it's a conscious choice and not a surprise down the road.
25-29: 🩺 Stability & AvailabilityThe new pure virtuals are already implemented in
StorageReaderandStorage_Read_Container— no compile break for these classes.Both
StorageReader(lines 27–31 instorage_reader.hpp) andStorage_Read_Container(lines 25, 28 instorage_read_container.hpp) correctly overrideprime,listIndices, andentries. The concern about these specific implementations causing a build failure is resolved.If there are other concrete subclasses (e.g., the ROOT-specific containers mentioned previously) not visible in this search, they must also provide overriding definitions to avoid ABI/compile errors.
form/storage/storage_read_container.hpp (1)
25-28: LGTM!form/root_storage/root_rfield_read_container.hpp (1)
35-37: LGTM!form/root_storage/root_tbranch_read_container.hpp (1)
23-26: LGTM!form/root_storage/root_tbranch_read_container.cpp (1)
19-25: LGTM!Single global mutex serializing all TBranch I/O is the right call given
TTree/ROOT global state isn't thread-safe — no notes there.form/storage/storage_writer.cpp (2)
12-44: These helpers are the other half of the duplication I flagged instorage_reader.cpp:17-48— same fix applies (extract to a shared header). Calling it out here for visibility rather than re-litigating it.
70-97: LGTM!form/form/form_reader.cpp (1)
37-49: LGTM!form/persistence/persistence_reader.hpp (1)
38-44: LGTM!form/persistence/persistence_reader.cpp (1)
51-65: LGTM!form/storage/storage_read_container.cpp (1)
29-38: LGTM!form/storage/storage_reader.hpp (1)
27-31: LGTM!form/storage/storage_reader.cpp (1)
277-302: LGTM!
| auto const trimmed = component.substr(first, component.find_last_not_of(' ') - first + 1); | ||
| auto const colon = trimmed.find(':'); | ||
| if (colon == std::string::npos) { | ||
| throw std::runtime_error("Unsupported FORM index component: " + trimmed); | ||
| } | ||
| std::string const layer = trimmed.substr(0, colon); | ||
| std::size_t const number = std::stoull(trimmed.substr(colon + 1)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
std::stoull will throw something cryptic on malformed input.
If the number portion isn't actually numeric (or the layer is empty because : sits at position 0), this either throws a bare std::invalid_argument/std::out_of_range or builds a child with an empty layer—neither matches the friendly runtime_error messages you raise everywhere else in this parser. Worth tightening so a bad FORM index string fails loudly and consistently.
🛠️ Suggested guard
std::string const layer = trimmed.substr(0, colon);
- std::size_t const number = std::stoull(trimmed.substr(colon + 1));
+ if (layer.empty()) {
+ throw std::runtime_error("Unsupported FORM index component (empty layer): " + trimmed);
+ }
+ std::size_t pos = 0;
+ std::size_t number = 0;
+ try {
+ number = std::stoull(trimmed.substr(colon + 1), &pos);
+ } catch (std::exception const&) {
+ throw std::runtime_error("Unsupported FORM index component: " + trimmed);
+ }
current = current->make_child(layer, number);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto const trimmed = component.substr(first, component.find_last_not_of(' ') - first + 1); | |
| auto const colon = trimmed.find(':'); | |
| if (colon == std::string::npos) { | |
| throw std::runtime_error("Unsupported FORM index component: " + trimmed); | |
| } | |
| std::string const layer = trimmed.substr(0, colon); | |
| std::size_t const number = std::stoull(trimmed.substr(colon + 1)); | |
| auto const trimmed = component.substr(first, component.find_last_not_of(' ') - first + 1); | |
| auto const colon = trimmed.find(':'); | |
| if (colon == std::string::npos) { | |
| throw std::runtime_error("Unsupported FORM index component: " + trimmed); | |
| } | |
| std::string const layer = trimmed.substr(0, colon); | |
| if (layer.empty()) { | |
| throw std::runtime_error("Unsupported FORM index component (empty layer): " + trimmed); | |
| } | |
| std::size_t pos = 0; | |
| std::size_t number = 0; | |
| try { | |
| number = std::stoull(trimmed.substr(colon + 1), &pos); | |
| } catch (std::exception const&) { | |
| throw std::runtime_error("Unsupported FORM index component: " + trimmed); | |
| } | |
| current = current->make_child(layer, number); |
🤖 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/form_source.cpp` around lines 40 - 46, The FORM index parsing in the
component handling logic needs explicit validation before calling std::stoull so
malformed input does not surface as a cryptic exception or allow an empty layer.
Update the parsing in the code path that extracts layer and number from trimmed
to check that the colon is neither missing nor at position 0, verify the number
substring is non-empty and numeric, and then raise a std::runtime_error with a
clear message when the input is invalid. Keep the behavior consistent with the
existing parser’s friendly error handling around FORM index components.
| phlex::index_generator indices() override | ||
| { | ||
| if (products_.empty()) { | ||
| co_return; | ||
| } | ||
|
|
||
| for (auto const& index_string : reader_->indices(actual_creator_, products_.front())) { | ||
| co_yield parse_index_string(index_string); | ||
| } | ||
| return *static_cast<T const*>(pb.data); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any contract/comment guaranteeing a shared index space across products,
# and how listIndices/indices is keyed.
rg -nP -C3 '\b(listIndices|indices)\s*\(' form/
ast-grep run --pattern 'std::vector<std::string> $_::listIndices($$$) { $$$ }' --lang cpp form/Repository: Framework-R-D/phlex
Length of output: 5904
indices() iterates only the first product, but the backend ignores the product name anyway
The product_name argument in reader_->indices() is discarded in persistence_reader.cpp (lines 77-80), where the lookup key is hard-coded to "index". Consequently, indices(actual_creator_, products_.front()) returns the same result as indices(actual_creator_, products_[N]).
The "silent data loss" fear is unfounded: adding an outer loop over products_ would be redundant, not a fix. The real question is architectural intent—do all products share a single index space? If yes, the current code is functionally correct despite the misleading signature. If no, the product_name must be wired into the storage key.
Confirm whether multi-product distinct indices are required; if so, fix persistence_reader.cpp to include the product in the lookup key.
🤖 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/form_source.cpp` around lines 116 - 125, The current `indices()`
implementation in `form_source.cpp` only passes `products_.front()` into
`reader_->indices()`, but the comment indicates the backend in
`persistence_reader.cpp` ignores that argument and always looks up the
hard-coded `"index"` key. Confirm the intended model for
`reader_->indices(actual_creator_, ...)` and `persistence_reader`—if
product-specific indices are required, wire `product_name` into the storage
lookup key in `persistence_reader.cpp`; otherwise keep the current single shared
index behavior and update the signature/comments to reflect that `product_name`
is intentionally unused.
| std::string actual_creator = advertised_creator; | ||
| try { | ||
| std::string const algorithm = config.get<std::string>("algorithm"); | ||
| std::string const plugin = config.get<std::string>("plugin"); | ||
| if (!algorithm.empty() && !plugin.empty()) { | ||
| actual_creator = plugin + ":" + algorithm; | ||
| } | ||
| } catch (...) { | ||
| // Keep advertised creator when algorithm/plugin are not provided. | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
catch (...) is a bigger net than you want here.
This swallows every exception, not just "key not found." A genuinely malformed algorithm/plugin value (or any unexpected throw) silently degrades to advertised_creator with zero signal. Since config.get already supports defaults, you can express "optional" without the catch-all and keep real errors visible.
♻️ Default-based alternative
- std::string actual_creator = advertised_creator;
- try {
- std::string const algorithm = config.get<std::string>("algorithm");
- std::string const plugin = config.get<std::string>("plugin");
- if (!algorithm.empty() && !plugin.empty()) {
- actual_creator = plugin + ":" + algorithm;
- }
- } catch (...) {
- // Keep advertised creator when algorithm/plugin are not provided.
- }
+ std::string actual_creator = advertised_creator;
+ std::string const algorithm = config.get<std::string>("algorithm", "");
+ std::string const plugin = config.get<std::string>("plugin", "");
+ if (!algorithm.empty() && !plugin.empty()) {
+ actual_creator = plugin + ":" + algorithm;
+ }Heads up: this assumes the two-arg config.get<T>(key, default) overload exists (you already rely on it for technology/module_label), so it should be a clean swap.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::string actual_creator = advertised_creator; | |
| try { | |
| std::string const algorithm = config.get<std::string>("algorithm"); | |
| std::string const plugin = config.get<std::string>("plugin"); | |
| if (!algorithm.empty() && !plugin.empty()) { | |
| actual_creator = plugin + ":" + algorithm; | |
| } | |
| } catch (...) { | |
| // Keep advertised creator when algorithm/plugin are not provided. | |
| } | |
| std::string actual_creator = advertised_creator; | |
| std::string const algorithm = config.get<std::string>("algorithm", ""); | |
| std::string const plugin = config.get<std::string>("plugin", ""); | |
| if (!algorithm.empty() && !plugin.empty()) { | |
| actual_creator = plugin + ":" + algorithm; | |
| } |
🤖 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/form_source.cpp` around lines 158 - 167, The creator resolution logic in
form_source.cpp is too broad because the catch-all around config.get for
algorithm and plugin hides real failures and silently falls back to
advertised_creator. Update the actual_creator assignment to use the existing
default-based config.get<T>(key, default) overload, similar to the
technology/module_label pattern, so missing keys are treated as optional without
swallowing unexpected exceptions. Keep the actual_creator/plugin:algorithm
composition in place, but remove the catch (...) block so malformed values and
other real errors remain visible.
| #include "form_source_type_registry.hpp" | ||
|
|
||
| #include <mutex> | ||
| #include <unordered_map> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd form_source_type_registry.hpp --exec sh -c 'echo "== {} =="; grep -nE "`#include` <(stdexcept|string|typeinfo)>" "{}" || echo " (none found directly)"'Repository: Framework-R-D/phlex
Length of output: 250
Pin direct dependencies: <stdexcept>, <string>, and <typeinfo> in form/form/form_source_type_registry.cpp.
The header form/form/form_source_type_registry.hpp already includes <stdexcept> and <string>, so transitive compilation currently works for those. However, the implementation file uses std::type_info (likely via typeid), and <typeinfo> is not guaranteed by the header or other includes.
Strict IWYU (Include What You Use) dictates that form/form/form_source_type_registry.cpp should declare every standard library symbol it uses. Relying on the header to pull in <typeinfo> is brittle; if the header changes, your build breaks silently on some toolchains or explicitly on others.
Add these headers to form/form/form_source_type_registry.cpp to lock in the dependency.
<details>
<summary>♻️ Add the direct includes</summary>
`#include` "form/form/form_source_type_registry.hpp"
+#include <stdexcept>
+#include <string>
+#include <typeinfo>
`#include` <mutex>
`#include` <unordered_map></details>
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include "form_source_type_registry.hpp" | |
| #include <mutex> | |
| #include <unordered_map> | |
| `#include` "form_source_type_registry.hpp" | |
| `#include` <stdexcept> | |
| `#include` <string> | |
| `#include` <typeinfo> | |
| `#include` <mutex> | |
| `#include` <unordered_map> |
🤖 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/form/form_source_type_registry.cpp` around lines 3 - 6,
form_source_type_registry.cpp is relying on transitive includes for standard
library symbols it uses. Add the missing direct standard headers for
std::type_info/typeid as well as the std::string and stdexcept dependencies to
form_source_type_registry.cpp, keeping the existing
form_source_type_registry.hpp usage intact and making the implementation file
self-sufficient.
| form_source_type_entry const* find_form_product_type(std::string const& product_type) | ||
| { | ||
| ensure_builtin_form_product_types_registered(); | ||
|
|
||
| std::lock_guard<std::mutex> lock(form_type_registry_mutex()); | ||
| auto const& registry = mutable_form_type_registry(); | ||
| auto const it = registry.find(product_type); | ||
| if (it == registry.end()) { | ||
| return nullptr; | ||
| } | ||
| return &it->second; | ||
| } | ||
|
|
||
| std::string const* find_form_product_type_name(phlex::experimental::type_id const& type) | ||
| { | ||
| ensure_builtin_form_product_types_registered(); | ||
|
|
||
| std::lock_guard<std::mutex> lock(form_type_registry_mutex()); | ||
| auto const& registry = mutable_form_type_registry(); | ||
| for (auto const& [name, entry] : registry) { | ||
| if (entry.type_id == type) { | ||
| return &name; | ||
| } | ||
| } | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Are there any registration calls outside startup/once paths?
rg -nP '\bregister_form_(product|vector)_type\b' -g '!*_test.cpp' -C2Repository: Framework-R-D/phlex
Length of output: 157
🏁 Script executed:
#!/bin/bash
# Broader search for register_form patterns
rg -n 'register_form' -g '!*_test.cpp'Repository: Framework-R-D/phlex
Length of output: 157
🏁 Script executed:
#!/bin/bash
# Locate the actual registration function definitions to understand visibility
rg -n 'register.*type.*{' -g 'form/**/*.cpp' | head -20Repository: Framework-R-D/phlex
Length of output: 255
🏁 Script executed:
#!/bin/bash
# Search for function definition patterns
rg -n '^[[:space:]]*(?:static|inline|.*::)register_form.*type.*\(.*\)[[:space:]]*\{' -g 'form/**/*.cpp' -g 'form/**/*.hpp'Repository: Framework-R-D/phlex
Length of output: 157
🏁 Script executed:
#!/bin/bash
# First, let's find the registration functions by searching for function declarations
rg -n 'register_form' --type cpp --type hpp | head -30Repository: Framework-R-D/phlex
Length of output: 185
🏁 Script executed:
#!/bin/bash
# Search for "register_form" in C++ files with broader context
rg -n 'register_form' -t cpp -t h -C3Repository: Framework-R-D/phlex
Length of output: 157
🏁 Script executed:
#!/bin/bash
# Search all C++ and header files for "register_form" without file type restrictions
rg -n 'register_form' --glob '!*_test.cpp' --glob '!*.md'Repository: Framework-R-D/phlex
Length of output: 157
🏁 Script executed:
#!/bin/bash
# Check the actual file content to see what functions exist
cat -n form/form/form_source_type_registry.cpp | grep -A5 -B5 "register"Repository: Framework-R-D/phlex
Length of output: 3154
🏁 Script executed:
#!/bin/bash
# Check where register_form_product_type and register_form_vector_product_type are declared
rg -n 'register_form_product_type' -g '!*_test.cpp' -B2 -A2Repository: Framework-R-D/phlex
Length of output: 157
🏁 Script executed:
#!/bin/bash
# Find header files declaring these functions
find . -name "*.hpp" -o -name "*.h" | xargs rg -n 'register_form' 2>/dev/null | head -20Repository: Framework-R-D/phlex
Length of output: 706
🏁 Script executed:
#!/bin/bash
# Check the header file for function declarations and visibility
cat -n form/form/form_source_type_registry.hppRepository: Framework-R-D/phlex
Length of output: 3509
🏁 Script executed:
#!/bin/bash
# Check usage in form/form_source.cpp around line 80
sed -n '75,85p' form/form/form_source.cppRepository: Framework-R-D/phlex
Length of output: 226
🏁 Script executed:
#!/bin/bash
# Find the correct path for form_source.cpp
find . -name "form_source.cpp" -type f 2>/dev/nullRepository: Framework-R-D/phlex
Length of output: 180
🏁 Script executed:
#!/bin/bash
# Check usage in form/form_source.cpp around line 80
sed -n '75,85p' form/form_source.cppRepository: Framework-R-D/phlex
Length of output: 557
Heads-up: find_* returns pointers that outlive the lock.
find_form_product_type (returns &it->second) and find_form_product_type_name (returns &name) release the registry lock before returning. While re-registering an existing key would cause a data race on the map node, verification shows register_form_product_type is currently never called in non-test production code; built-in types are registered via std::call_once at startup.
Since the API sits in form::experimental and assumes init-time registration, the current usage is safe. However, because the functions are public and return pointers to map internals, any future late registration would race.
- Action: Add a static assert or
[[nodiscard]]comment documenting that registration must complete before any lookup, or move registration behind an internal guard to prevent late calls.
🧰 Tools
🪛 Cppcheck (2.21.0)
[style] 42-42: The function 'find_form_product_type' is never used.
(unusedFunction)
[style] 55-55: The function 'find_form_product_type_name' is never used.
(unusedFunction)
🤖 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/form/form_source_type_registry.cpp` around lines 42 - 67, The lookup
helpers in form_source_type_registry.cpp return pointers into the registry after
releasing the mutex, so they are only safe if registration is complete before
any lookup. Update find_form_product_type and find_form_product_type_name to
make this contract explicit by documenting the init-time-only assumption near
register_form_product_type/ensure_builtin_form_product_types_registered, or
enforce it with an internal guard that prevents late registration after lookups
begin. Keep the fix focused on the registry APIs and their pointer-returning
behavior.
| if (all_components_zero(*normalized_query)) { | ||
| auto const empty_key = m_indexMaps[token.containerName()].find(""); | ||
| if (empty_key != m_indexMaps[token.containerName()].end()) { | ||
| return 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm how getIndex's return value is consumed and whether 0 is a valid/sentinel row id.
rg -nP -C4 '\bgetIndex\s*\(' form/
# Inspect getToken to see how rowId (getIndex result) is forwarded into the Token / read path.
ast-grep run --pattern 'std::unique_ptr<Token> PersistenceReader::getToken($$$) { $$$ }' --lang cpp form/persistence/persistence_reader.cpp
# Check whether read containers treat id 0 specially (entries numbered from 1?).
rg -nP -C3 '\bentries\s*\(\)|GetNEntries|id\s*[<>=]' form/root_storage/Repository: Framework-R-D/phlex
Length of output: 9612
Return the matched entry for the empty-string key instead of 0
In form/storage/storage_reader.cpp:261–266, when the normalized query is all-zeros and an empty-string index "" exists, the code finds empty_key but returns 0 instead of empty_key->second.
The normal match path in getIndex returns the mapped entry value (entry), yet this all-zeros path discards it and returns a sentinel 0. Downstream, persistence_reader.cpp passes this ID directly to Token, and read implementations check bounds against GetNEntries() without treating 0 specially as "no entry." This inconsistency likely causes a read of the wrong row (the first entry) or an invalid access if the empty key maps to a non-zero index.
Unless there's an explicit contract that 0 acts as a "default/empty" sentinel here (which isn't documented), return the found index:
return empty_key->second;Add a comment if 0 is intentionally special to avoid future confusion.
🤖 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/storage_reader.cpp` around lines 261 - 266, The all-zero
normalized query branch in getIndex is returning a sentinel 0 even when the
empty-string key is found, which diverges from the normal lookup path that
returns the mapped entry value. Update the storage_reader.cpp logic in getIndex
so that, after finding empty_key in m_indexMaps[token.containerName()], it
returns empty_key->second instead of 0, and add a short comment only if 0 is
meant to be a special contract.
| plugin: 'add_standard_and_offset_wires', | ||
| products: ['sums'], | ||
| creator: 'add_standard_and_offset_wires', | ||
| //advertised_creator: 'add_standard_and_offset_wires', // new form source |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
These // comments will trip the same jsonnetfmt --test check that nailed the write fixture.
CI normalizes comments to # (the sibling form_gaussian_write_test.jsonnet got converted), so these //advertised_creator lines will fail formatting. Either convert them to # or drop the commented-out lines entirely if they're just future-feature placeholders.
🧹 Convert to hash-style comments
creator: 'add_standard_and_offset_wires',
- //advertised_creator: 'add_standard_and_offset_wires', // new form source
+ # advertised_creator: 'add_standard_and_offset_wires', # new form source creator: 'add_standard_and_wider_wires',
- //advertised_creator: 'add_standard_and_wider_wires', // new form source
+ # advertised_creator: 'add_standard_and_wider_wires', # new form sourceAs per coding guidelines: "Use jsonnetfmt for consistent Jsonnet formatting; CI enforces this."
Also applies to: 25-25
🤖 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/form_gaussian_read_test.jsonnet` at line 16, The Jsonnet fixture
comment style is inconsistent with the formatter expected by CI, and the
`advertised_creator` placeholder comments in the form Gaussian read test will
fail `jsonnetfmt --test`. Update the commented placeholder lines in
`form_gaussian_read_test.jsonnet` to use hash-style comments like the matching
`form_gaussian_write_test.jsonnet`, or remove the placeholders entirely if they
are not needed, so the fixture stays formatter-compliant.
Sources: Coding guidelines, Pipeline failures
| @@ -0,0 +1,46 @@ | |||
| local wire_size = 1024; # ~600MB file in afew seconds with 4 threads on a GPVM | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Tiny typo: "afew" → "a few".
Spaces are free, friend. 🙂
📝 Fix
-local wire_size = 1024; # ~600MB file in afew seconds with 4 threads on a GPVM
+local wire_size = 1024; # ~600MB file in a few seconds with 4 threads on a GPVMSide note: the CI jsonnetfmt --test log flagged this file for //→# comment-style normalization. The version here already uses #, so that part looks settled—just make sure the latest commit is what CI re-runs against.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| local wire_size = 1024; # ~600MB file in afew seconds with 4 threads on a GPVM | |
| local wire_size = 1024; # ~600MB file in a few seconds with 4 threads on a GPVM |
🧰 Tools
🪛 GitHub Actions: wwuoneway checking Jsonnet format / 0_jsonnet-format-check.txt
[error] 1-4: jsonnetfmt --test failed (formatting diff detected). FAILED: ./test/form/form_gaussian_write_test.jsonnet. Diff shows comment style change required: local wire_size = 1024; // ~600MB file... -> local wire_size = 1024; # ~600MB file....
🪛 GitHub Actions: wwuoneway checking Jsonnet format / jsonnet-format-check
[error] 1-1: jsonnetfmt --test failed (format mismatch). FAILED diff: comment style changed from '// ~600MB file in afew seconds with 4 threads on a GPVM' to '# ~600MB file in afew seconds with 4 threads on a GPVM'. Step command: jsonnetfmt --test "{}" || (echo "FAILED: {}" && diff -u <(jsonnetfmt "{}") "{}" && exit 1)
🤖 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/form_gaussian_write_test.jsonnet` at line 1, Fix the typo in the
comment on the wire_size declaration by changing “afew” to “a few” in
form_gaussian_write_test.jsonnet. Keep the existing # comment style as-is; the
only needed change is the wording in that comment.
Source: Pipeline failures
| #include "phlex/model/data_cell_index.hpp" | ||
| #include "phlex/module.hpp" | ||
| #include "phlex/source.hpp" | ||
|
|
||
| #include <chrono> | ||
| #include <random> | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Add the includes you're actually leaning on.
assert (line 56) needs <cassert>, and you use std::vector / size_t directly. Right now these only compile because some phlex header drags them in transitively—classic house-of-cards that breaks the moment an upstream include gets cleaned up. Include what you use.
🧱 Suggested includes
`#include` "phlex/model/data_cell_index.hpp"
`#include` "phlex/module.hpp"
`#include` "phlex/source.hpp"
+#include <cassert>
`#include` <chrono>
+#include <cstddef>
`#include` <random>
+#include <vector>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include "phlex/model/data_cell_index.hpp" | |
| #include "phlex/module.hpp" | |
| #include "phlex/source.hpp" | |
| #include <chrono> | |
| #include <random> | |
| `#include` "phlex/model/data_cell_index.hpp" | |
| `#include` "phlex/module.hpp" | |
| `#include` "phlex/source.hpp" | |
| `#include` <cassert> | |
| `#include` <chrono> | |
| `#include` <cstddef> | |
| `#include` <random> | |
| `#include` <vector> |
🤖 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/generate_vector.cpp` around lines 1 - 7, The test file relies on
transitive includes for standard library symbols, so make its dependencies
explicit. Add the missing standard headers needed by the uses in the test,
especially the header for assert and any headers required for std::vector and
size_t, and keep the existing phlex includes unchanged. Check the includes at
the top of generate_vector.cpp and ensure every directly used type or macro is
covered by its own include.
| int const seed = | ||
| config.get<int>("seed", std::chrono::system_clock::now().time_since_epoch().count()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Seed default narrows a 64-bit count into int.
time_since_epoch().count() is a 64-bit rep; feeding it into get<int> truncates it (and likely trips -Wconversion). It still "seeds something," but the top bits are gone and the value can go negative. Cast it explicitly to make intent obvious and quiet the warning.
🎲 Suggested tweak
- int const seed =
- config.get<int>("seed", std::chrono::system_clock::now().time_since_epoch().count());
+ int const seed = config.get<int>(
+ "seed",
+ static_cast<int>(std::chrono::system_clock::now().time_since_epoch().count()));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int const seed = | |
| config.get<int>("seed", std::chrono::system_clock::now().time_since_epoch().count()); | |
| int const seed = config.get<int>( | |
| "seed", | |
| static_cast<int>(std::chrono::system_clock::now().time_since_epoch().count())); |
🤖 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/generate_vector.cpp` around lines 34 - 35, The seed default in
generate_vector uses time_since_epoch().count() as the fallback for
config.get<int>, which narrows a 64-bit value into int and can trigger
conversion warnings. Update the seed retrieval in generate_vector so the default
value is explicitly converted to int before passing it to config.get, making the
truncation intentional and clear.
Clang-Tidy Check Results4 new issue(s) introduced by this patch (5545 total). New issues on modified lines:
All issues by check:
See inline comments for details. Comment |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (58.43%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #674 +/- ##
==========================================
- Coverage 83.55% 82.59% -0.97%
==========================================
Files 170 172 +2
Lines 6283 6665 +382
Branches 706 801 +95
==========================================
+ Hits 5250 5505 +255
- Misses 810 881 +71
- Partials 223 279 +56
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Clang-Tidy Check Results4 new issue(s) introduced by this patch (5545 total). New issues on modified lines:
All issues by check:
See inline comments for details. Comment |
Clang-Tidy Check Results4 new issue(s) introduced by this patch (5545 total). New issues on modified lines:
All issues by check:
See inline comments for details. Comment |
Clang-Tidy Check Results4 new issue(s) introduced by this patch (5582 total). New issues on modified lines:
All issues by check:
See inline comments for details. Comment |
Clang-Tidy Check Results4 new issue(s) introduced by this patch (5582 total). New issues on modified lines:
All issues by check:
See inline comments for details. Comment |
wwuoneway
requested review from
aolivier23,
gemmeren,
knoepfel,
marcpaterno and
pcanal
Clang-Tidy Check ResultsFound 5580 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
Clang-Tidy Check ResultsFound 5606 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
gemmeren
left a comment
gemmeren
left a comment
There was a problem hiding this comment.
I don't know about:
codecov/patchFailing after 1s — 58.43% of diff hit (target 80.00%)
and there are improvements to be done, but I don't have objections to merging this as is and applying those in later PRs.
3 participants
Summary:
Allow FORM to inherit the Phlex source and read FORM input files. As this stage, metadata support is not included in this PR.
Code / API
prime(...)andlistIndices(...)to the persistence reader and storage reader interfaces, with matching implementations across FORM and storage backends.Storage_Read_Container,ROOT_RField_Read_ContainerImp,ROOT_TBranch_Read_ContainerImp) withprime(...)andentries()support.FormInputSourcesource implementation andPHLEX_REGISTER_SOURCE, with support formodule_labelandactual_creatorresolution.phlex::model.Tests
Build / Test infra
generate_vectorandform_source_extra_typestest modules to the FORM test CMake setup.