Issue: 4556521 Show all non-default config variables programmatically #502
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Can one of the admins verify this patch? |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors config variable display from hardcoded per-variable logging to a generic registry-based iteration approach.
Key Changes:
- Made
config_registrypersist as a member variable inmce_sys_varinstead of being a local variable, enabling access throughout the process lifetime - Added public accessors to expose
config_descriptorand parameter map for iteration - Added
titlefield to parameter descriptors, extracted from JSON schema - Replaced ~300 lines of hardcoded VLOG calls in
main.cppwith a generic loop that iterates over all registered parameters - Implemented
print_config_element()for recursive, type-aware config printing (handles bool, string, int64_t, vectors, and maps) - Added
convert_int64_to_mapped_string()for reverse enum lookup to display enum values as strings - Improved error messages for invalid enum values by catching
std::runtime_errorand including valid options - Added
option_size::to_str_accurate()for size formatting without rounding (e.g., 4096 → "4K", not "4K" from rounded 4097)
Benefits:
- New config parameters are automatically displayed without code changes in
main.cpp - Eliminates risk of forgetting to add display code for new parameters
- More maintainable and DRY approach
- Better error messages for invalid enum values
Issues Found:
- Debug output lines (
BEGIN OREN/END OREN) should be removed (lines 963, 1040 inmain.cpp) - Commented-out code and "DELETE ME" comment in
config_registry.hshould be cleaned up - Minor indentation issue in
config_registry.cppline 124 - Placeholder title "XYZ-TITLE-NOT-SET" in schema analyzer could be improved
Confidence Score: 4/5
- This PR is safe to merge after removing debug output lines and cleaning up commented code.
- Score reflects solid refactoring with good test coverage, but debug artifacts and commented code need cleanup before merge. The core logic is sound - replacing hardcoded logging with generic iteration is a clear improvement. Tests validate the functionality works correctly. No critical bugs found, only style issues.
- Pay attention to
src/core/main.cpp(remove debug lines 963, 1040) andsrc/core/config/config_registry.h(clean up commented code and "DELETE ME" comment). Minor fix needed insrc/core/config/config_registry.cppline 124 for indentation.
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/core/config/config_registry.h | 4/5 | Simplified template methods by removing SFINAE specialization and made get_config_descriptor() public. Contains leftover "DELETE ME" comment and commented-out code that should be cleaned up. |
| src/core/config/config_registry.cpp | 4/5 | Added new catch block for std::runtime_error to improve error messages for invalid enum values. Has indentation issue on line 124. |
| src/core/config/descriptor_providers/schema_analyzer.cpp | 4/5 | Added determine_title() method to extract title from JSON schema. Uses placeholder value "XYZ-TITLE-NOT-SET" when title not found. |
| src/core/config/descriptors/parameter_descriptor.cpp | 4/5 | Added title getter/setter, improved enum validation error messages, and added convert_int64_to_mapped_string() for reverse enum lookup. The "(Invalid value)" suffix may be misleading for valid unmapped integers. |
| src/core/main.cpp | 4/5 | Replaced ~300 lines of hardcoded config variable logging with generic registry iteration in print_xlio_global_settings(). Added print_config_element() for recursive type-aware printing. Contains debug output lines that should be removed. |
| src/core/util/sys_vars.cpp | 5/5 | Changed registry from local variable to member variable m_registry, and added option_size::to_str_accurate() for accurate size formatting without rounding. |
Sequence Diagram
sequenceDiagram
participant Main as main.cpp::print_xlio_global_settings()
participant SysVars as mce_sys_var
participant Registry as config_registry
participant Descriptor as config_descriptor
participant ParamDesc as parameter_descriptor
participant PrintFunc as print_config_element()
Main->>SysVars: get_registry()
SysVars-->>Main: config_registry&
Main->>Registry: get_config_descriptor()
Registry-->>Main: config_descriptor&
Main->>Descriptor: get_parameter_map()
Descriptor-->>Main: parameter_map_t&
loop For each parameter in parameter_map
Main->>ParamDesc: get_title()
ParamDesc-->>Main: title string
alt Special parameter (log_level, rx_num_wre, etc.)
Main->>Main: Handle with custom logic
Main->>Main: VLOG_PARAM_*() macro
else Generic parameter
Main->>Registry: get_value_as_any(key)
Registry-->>Main: std::experimental::any
Main->>ParamDesc: default_value()
ParamDesc-->>Main: default std::experimental::any
Main->>PrintFunc: print_config_element(key, descriptor, value, default)
alt Type is bool/string/int64_t
PrintFunc->>PrintFunc: any_cast to concrete type
alt int64_t with string mapping
PrintFunc->>ParamDesc: convert_int64_to_mapped_string(value)
ParamDesc-->>PrintFunc: mapped string or empty
end
PrintFunc->>PrintFunc: VLOG_PARAM_*() macro
else Type is vector
loop For each vector element (max 5)
PrintFunc->>PrintFunc: Recursive call for element
end
else Type is map
loop For each map entry
PrintFunc->>PrintFunc: Recursive call for entry
end
end
end
end
Additional Comments (1)
-
src/core/config/descriptors/schema_analyzer.cpp, line 171 (link)style: hardcoded placeholder title
"XYZ-TITLE-NOT-SET"- consider using a more descriptive fallback like the parameter key itself, or throwing an exception if title is required
16 files reviewed, 6 comments
tomerdbz
requested changes
Nov 2, 2025
Collaborator
tomerdbz
left a comment
tomerdbz
left a comment
There was a problem hiding this comment.
Incredible work Oren. Some comments and blockers - but we're not far merging this! :)
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The changes include minor cleanup work addressing previous review feedback. The main updates are: removal of debug output markers from main.cpp, cleanup of a commented-out line in the unit test, and a minor fallback title string change in the schema analyzer. These are all cosmetic improvements that don't affect functionality.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| src/core/config/descriptor_providers/schema_analyzer.cpp | 5/5 | Changed fallback title string from "XYZ-TITLE-NOT-SET" to "TITLE-NOT-SET" |
| tests/unit_tests/config/config_registry.cpp | 5/5 | Removed commented-out assertion line in unit test |
| src/core/config/config_registry.cpp | 5/5 | Fixed indentation alignment for closing brace |
| src/core/config/config_registry.h | 4/5 | Major simplification of template interface and removal of extensive commented code |
| src/core/main.cpp | 5/5 | Removed debug output markers "BEGIN OREN" and "END OREN" |
Confidence score: 4/5
- This PR addresses minor cleanup items from previous reviews and appears safe to merge with minimal risk
- Score reflects mostly cosmetic changes with one significant interface simplification in config_registry.h that removes SFINAE complexity while maintaining functionality through existing template specializations
- The config_registry.h changes require attention to ensure the simplified interface maintains all necessary bounds checking for integer types
5 files reviewed, 1 comment
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors config display from hardcoded parameter printing to dynamic iteration over the config registry, eliminating the need to manually add code for each new parameter.
Key Changes:
- Extended
config_registrylifetime to process scope by storing as member inmce_sys_var - Added
get_parameter_map()accessor to iterate all config parameters - Implemented title extraction from schema and
convert_int64_to_mapped_string()for display - Replaced 300+ lines of hardcoded
VLOG_PARAM_*calls with generic iteration logic - Added
to_str_accurate()utility for K/M/G suffix formatting without rounding
Critical Issue Found:
convert_int64_to_mapped_string()throws unhandled exception when int64 value isn't in string mapping, causing startup crash inprint_int64_config_element()(main.cpp:867-868)
Confidence Score: 1/5
- Critical crash bug prevents safe merge - application will crash at startup when displaying certain config values
- Score reflects critical unhandled exception in
convert_int64_to_mapped_string()that will crash at startup. The function throwsruntime_errorfor unmapped int64 values, but callers in main.cpp lines 867-868 have no exception handling. This will trigger whenever a parameter has string mappings but current/default value isn't in the mapping. - src/core/config/descriptors/parameter_descriptor.cpp (critical bug) and src/core/main.cpp (missing exception handling)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/core/config/descriptors/parameter_descriptor.cpp | 1/5 | Critical bug: convert_int64_to_mapped_string() throws unhandled exception that will crash at startup |
| src/core/main.cpp | 2/5 | Refactored config display to iterate registry dynamically; missing exception handling for convert_int64_to_mapped_string calls (lines 867-868) |
| src/core/config/config_registry.h | 4/5 | Added get_config_descriptor() accessor and made get_value_as_any() public for iteration support |
| src/core/util/sys_vars.h | 5/5 | Added m_registry member and get_registry() accessor; added to_str_accurate() utility function |
| src/core/config/descriptors/parameter_descriptor.h | 4/5 | Added title support (get/set methods) and convert_int64_to_mapped_string() for display purposes |
Sequence Diagram
sequenceDiagram
participant App as Application (main)
participant Sys as mce_sys_var
participant Reg as config_registry
participant Desc as config_descriptor
participant Param as parameter_descriptor
participant Print as print_config_element
Note over App,Print: Startup: Config Initialization
App->>Sys: construct mce_sys_var
Sys->>Reg: construct config_registry (m_registry)
Reg->>Desc: load_descriptors()
Desc-->>Reg: config_descriptor with parameter_map
Note over Reg: Registry now lives for process lifetime
Note over App,Print: Display: Print Config Values
App->>Sys: safe_mce_sys().get_registry()
Sys-->>App: config_registry reference
App->>Reg: get_config_descriptor()
Reg-->>App: config_descriptor
App->>Desc: get_parameter_map()
Desc-->>App: parameter_map (all parameters)
loop For each parameter in map
App->>Param: get_title()
Param-->>App: title string
App->>Reg: get_value_as_any(key)
Reg-->>App: current value
App->>Param: default_value()
Param-->>App: default value
App->>Print: print_config_element(key, descriptor, value, default)
alt int64 type with string mapping
Print->>Param: convert_int64_to_mapped_string(value)
alt value in mapping
Param-->>Print: mapped string
else value not in mapping
Param->>Print: throw runtime_error ❌
Note over Print: CRASH - No exception handling!
end
else other types
Print->>Print: print directly
end
end
14 files reviewed, 1 comment
orshemesh-grok
force-pushed
the
redmine-4556521
branch
from
2582b10 to
1ef86d6
Compare
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors configuration variable display at startup from manual per-variable code to a generic registry-scanning approach. The main architectural change makes the config_registry persist as a member variable (m_registry) in mce_sys_var throughout the process lifetime, allowing main.cpp to iterate over all registered parameters and display them dynamically.
Key changes:
- Replaced ~320 lines of manual VLOG_PARAM calls with a loop that iterates the parameter map and delegates to type-specific print functions
- Added
parameter_descriptor::convert_int64_to_mapped_string()to display enum-like integers as their string representations - Extended schema analysis to capture and enforce
titlefield from JSON schema - Added
option_size::to_str_accurate()to format sizes with K/M/G suffixes without rounding - Special parameters that need non-standard logic remain handled via a
special_treatmentsmap with lambdas
Critical issue identified:
The previous review comment regarding convert_int64_to_mapped_string() throwing exceptions at display time (lines 867-868 in main.cpp) without try-catch handling is valid and unresolved. This function throws std::runtime_error when an int64 value doesn't have a string mapping, but this happens during startup display, not during config validation. While config loading has exception handling (config_registry.cpp:120), the display code path does not, which could cause crashes when displaying parameters with partial mappings or when default values fall outside the mapped range.
Confidence Score: 3/5
- This PR has a critical unresolved issue where exception-throwing code is called without try-catch during startup display
- The refactoring approach is sound and the architecture is cleaner, but the unresolved exception handling issue in convert_int64_to_mapped_string() could cause startup crashes. The previous review correctly identified this problem, and it remains unfixed - the function still throws exceptions for unmapped values at display time rather than returning empty strings
- src/core/config/descriptors/parameter_descriptor.cpp and src/core/main.cpp require immediate attention to handle the exception-throwing behavior
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/core/main.cpp | 3/5 | Replaced manual config variable printing with generic loop-based approach; relies on exception-throwing convert_int64_to_mapped_string without try-catch which could crash at startup |
| src/core/config/descriptors/parameter_descriptor.cpp | 2/5 | Added convert_int64_to_mapped_string that throws exception for unmapped values instead of returning empty string, causing potential crash at display time |
| src/core/config/config_registry.cpp | 5/5 | Added catch block for runtime_error during config loading to provide better error messages |
| src/core/util/sys_vars.cpp | 5/5 | Made config_registry a member variable (m_registry) to persist throughout process lifetime, added to_str_accurate for K/M/G suffix formatting |
Sequence Diagram
sequenceDiagram
participant Main as main.cpp
participant SysVars as mce_sys_var
participant Registry as config_registry
participant Descriptor as config_descriptor
participant ParamDesc as parameter_descriptor
Note over Main: Startup: print_xlio_global_settings()
Main->>SysVars: safe_mce_sys().get_registry()
SysVars-->>Main: config_registry reference
Main->>Registry: get_config_descriptor()
Registry-->>Main: config_descriptor
Main->>Descriptor: get_parameter_map()
Descriptor-->>Main: parameter_map (all config params)
loop For each parameter
Main->>Main: Check special_treatments map
alt Special treatment exists
Main->>Main: Execute custom display logic
else Generic handling
Main->>Registry: get_value_as_any(key)
Registry-->>Main: current value as any
Main->>ParamDesc: default_value()
ParamDesc-->>Main: default value as any
Main->>Main: print_config_element()
alt Type is int64_t
Main->>ParamDesc: convert_int64_to_mapped_string(def_value)
Note over ParamDesc: THROWS if value not in mapping!
ParamDesc-->>Main: mapped string or exception
Main->>ParamDesc: convert_int64_to_mapped_string(cur_value)
Note over ParamDesc: THROWS if value not in mapping!
ParamDesc-->>Main: mapped string or exception
alt String mapping found
Main->>Main: Display with string representation
else Empty string returned
Main->>Main: Display with numeric value
end
else Other types (bool, string, vector, map)
Main->>Main: Display with type-specific formatting
end
end
end
16 files reviewed, no comments
orshemesh-grok
force-pushed
the
redmine-4556521
branch
from
1ef86d6 to
ab3a8b2
Compare
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Refactors config variable display from hardcoded per-parameter code to automated registry scanning. This ensures all config parameters are shown at startup without requiring manual updates when new parameters are added.
Key Changes:
- Config registry lifecycle extended to process lifetime via
mce_sys_var::m_registrymember - New
print_config_element()family of functions iterate through all parameters programmatically parameter_descriptorgainstitlefield andconvert_int64_to_mapped_string()method for display- Special handling preserved for parameters with non-standard display requirements (log_level, rx_num_wre, progress_engine_interval, etc.)
- Helper function
option_size::to_str_accurate()added for K/M/G suffix display without rounding
Issues Found:
convert_int64_to_mapped_string()returns"N (Invalid value)"for unmapped integers, which will display misleading messages if default values aren't in string mappings. Should return empty string instead.
Confidence Score: 4/5
- This PR is generally safe to merge with one logic fix needed
- The refactoring is well-structured and maintains backward compatibility by preserving special handling for specific parameters. However, there's a logic bug in
convert_int64_to_mapped_string()that returns misleading "(Invalid value)" text for unmapped integers, which could confuse users during config display. This needs to be fixed before merge. - src/core/config/descriptors/parameter_descriptor.cpp needs the logic fix at line 266
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/core/main.cpp | 4/5 | Replaces hardcoded config display with registry-based loop. Adds helper functions for displaying different config types (int64, bool, string, vector, map). Special handling preserved for certain parameters. |
| src/core/config/config_registry.h | 5/5 | Makes config registry accessor methods public, adds get_config_descriptor() and makes get_value_as_any() public to support programmatic config display. Simplifies template methods. |
| src/core/config/descriptors/parameter_descriptor.cpp | 3/5 | Adds title field, convert_int64_to_mapped_string() method, and improves error messages. Has bug: returns "(Invalid value)" for unmapped integers instead of empty string. |
| src/core/util/sys_vars.cpp | 5/5 | Changes config_registry from local variable to member variable (m_registry). Adds to_str_accurate() helper for displaying sizes with K/M/G suffixes without rounding. |
| src/core/util/sys_vars.h | 5/5 | Adds m_registry member variable and get_registry() accessor to make config registry available throughout process lifetime. |
Sequence Diagram
sequenceDiagram
participant App as Application
participant Main as main.cpp
participant SysVars as mce_sys_var
participant Registry as config_registry
participant Descriptor as config_descriptor
participant ParamDesc as parameter_descriptor
App->>Main: startup/initialization
Main->>SysVars: safe_mce_sys()
SysVars->>Registry: initialize m_registry
Registry->>Descriptor: load config schema
Registry->>Registry: load config values
Main->>Main: print_xlio_global_settings()
Main->>SysVars: get_registry()
SysVars-->>Main: config_registry&
Main->>Registry: get_config_descriptor()
Registry-->>Main: config_descriptor&
Main->>Descriptor: get_parameter_map()
Descriptor-->>Main: parameter_map
loop for each parameter
Main->>Registry: get_value_as_any(key)
Registry-->>Main: current value
Main->>ParamDesc: get_title()
ParamDesc-->>Main: title string
Main->>ParamDesc: default_value()
ParamDesc-->>Main: default value
alt integer with string mapping
Main->>ParamDesc: convert_int64_to_mapped_string()
ParamDesc-->>Main: mapped string or "(Invalid value)"
end
Main->>Main: print_config_element()
Main->>Main: VLOG_PARAM_XXX (display)
end
16 files reviewed, 1 comment
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors configuration display logic to automatically show all non-default parameters by scanning the config registry, eliminating the need for manual per-parameter display code.
Key changes:
- Made
config_registrya member variable ofmce_sys_var(instead of local variable) to extend its lifetime to match the process - Exposed
get_config_descriptor()andget_parameter_map()methods to allow iteration over all config parameters - Replaced ~300 lines of manual
VLOG_PARAM_*calls inmain.cppwith a generic loop that handles all parameter types - Added helper functions
print_int64_config_element(),print_bool_config_element(),print_string_config_element()to display different types - Maintained special-case handling for parameters with complex display logic (e.g.,
CONFIG_VAR_LOG_LEVEL,CONFIG_VAR_PROGRESS_ENGINE_INTERVAL) - Added
convert_int64_to_mapped_string()to convert enum values to human-readable strings - Implemented
to_str_accurate()to format sizes with K/M/G suffixes only for exact powers of 1024
Architectural improvement:
The refactoring means new config parameters will be automatically displayed at startup without requiring developers to remember to add display code. This reduces maintenance burden and prevents accidentally missing parameters.
Issue identified in previous comments:
The convert_int64_to_mapped_string() function returns "N (Invalid value)" for unmapped values when mappings exist. This could be misleading if a parameter has partial string mappings (some values mapped, others intentionally not mapped). The display code should handle this case more gracefully.
Confidence Score: 4/5
- This PR is safe to merge with minor concerns around edge case handling in config display
- Score reflects well-architected refactoring with good test coverage, but the "(Invalid value)" display issue identified in previous comments should be addressed to avoid misleading output. The core changes (making registry a member variable, exposing iteration methods) are sound and don't introduce memory or lifecycle issues. Special-case handling is preserved for complex parameters.
- Pay attention to
src/core/config/descriptors/parameter_descriptor.cpp(the "(Invalid value)" return logic) and verify the test coverage intests/gtest/output/config.ccadequately exercises edge cases
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/core/config/config_registry.h | 4/5 | Exposed get_config_descriptor() method to allow external iteration over config parameters. Simplified template methods by removing separate integer/non-integer specializations. |
| src/core/config/descriptors/parameter_descriptor.cpp | 3/5 | Implemented new methods for parameter display. convert_int64_to_mapped_string() returns "(Invalid value)" suffix for unmapped values when mappings exist, which could be misleading if valid values are intentionally unmapped. Enhanced error messages in convert_string_to_int64(). |
| src/core/util/sys_vars.h | 5/5 | Added m_registry as member variable to extend config registry lifetime to match process lifetime. Added get_registry() method and to_str_accurate() function declaration. |
| src/core/util/sys_vars.cpp | 5/5 | Changed config registry from local to member variable (m_registry). Implemented to_str_accurate() to format sizes with K/M/G suffix only for exact powers of 1024, avoiding rounding. |
| src/core/main.cpp | 3/5 | Replaced manual config display code with generic loop over all parameters from registry. Added helper functions for printing different types. Special-case handling remains for parameters with complex display logic. Previous comments identified potential issues with "(Invalid value)" display for unmapped values. |
Sequence Diagram
sequenceDiagram
participant App as Application
participant MainInit as main.cpp (init)
participant SysVar as mce_sys_var
participant Registry as config_registry
participant Descriptor as config_descriptor
participant ParamDesc as parameter_descriptor
participant Display as print_xlio_global_settings()
App->>MainInit: LD_PRELOAD initialization
MainInit->>SysVar: Create mce_sys_var singleton
SysVar->>Registry: Initialize m_registry member
Registry->>Descriptor: Load parameter descriptors
Registry->>Registry: Load config values from sources
SysVar->>SysVar: apply_config_from_registry()
SysVar->>SysVar: Store config in member variables
Note over Registry: Registry lifetime now matches process lifetime
App->>Display: print_xlio_global_settings()
Display->>SysVar: get_registry()
SysVar-->>Display: return m_registry reference
Display->>Registry: get_config_descriptor()
Registry-->>Display: return descriptor
Display->>Descriptor: get_parameter_map()
Descriptor-->>Display: return parameter map
loop For each parameter
Display->>ParamDesc: get_title()
Display->>Registry: get_value_as_any(key)
Display->>ParamDesc: convert_int64_to_mapped_string() (if int64)
ParamDesc-->>Display: return string representation
Display->>Display: print_config_element()
Display->>Display: VLOG_PARAM_* macros
end
Note over Display: Special cases handled separately<br/>for complex display logic
17 files reviewed, no comments
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors configuration parameter display from hardcoded individual calls to automatic iteration through the registry, ensuring all parameters are shown without manual maintenance.
Key Changes:
- Config registry now persists throughout application lifetime (moved from local to member variable in
mce_sys_var) - Added
get_config_descriptor()accessor to enable programmatic parameter enumeration - Replaced ~300 lines of hardcoded
VLOG_PARAM_*calls inmain.cppwith generic iteration logic - Added
titlefield to parameter descriptors for display purposes - Implemented
convert_int64_to_mapped_string()for displaying enum values as strings - Added
option_size::to_str_accurate()for cleaner size formatting without rounding
Benefits:
- New parameters are automatically displayed without code changes
- Reduced maintenance burden and eliminated risk of forgetting to log new parameters
- Consistent handling across all parameter types (scalars, vectors, maps)
Previous Comments Addressed:
The PR author has already addressed style issues raised in previous comments (debug output lines, leftover comments, indentation, etc.).
Confidence Score: 4/5
- This PR is generally safe to merge with low risk, pending review of one potential display issue
- The refactoring is well-designed and reduces maintenance burden. The changes are mostly mechanical and follow good software engineering principles. Score is 4 (not 5) due to the "(Invalid value)" display behavior in parameter_descriptor.cpp that could be misleading, as noted in previous comments. All style issues from previous reviews appear to be addressed.
- src/core/config/descriptors/parameter_descriptor.cpp - review convert_int64_to_mapped_string() behavior for parameters with partial enum mappings
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/core/config/config_registry.h | 5/5 | Simplified template methods and added public accessor for config_descriptor. Changes are clean and improve API design by consolidating get_value methods. |
| src/core/main.cpp | 4/5 | Major refactoring to automatically display all config parameters by iterating through registry instead of hardcoded individual calls. Reduces code maintenance burden and ensures all parameters are shown. Some special cases still handled individually as needed. |
| src/core/config/descriptors/parameter_descriptor.cpp | 3/5 | Added title accessors, enhanced convert_string_to_int64 error messages, and implemented convert_int64_to_mapped_string. The "(Invalid value)" suffix in convert_int64_to_mapped_string may display misleading output for valid numeric values without string mappings. |
| src/core/util/sys_vars.h | 5/5 | Added registry member variable and public getter, plus new option_size::to_str_accurate utility. Clean changes that enable access to configuration registry throughout application lifecycle. |
| src/core/util/sys_vars.cpp | 5/5 | Changed registry from local to member variable and implemented to_str_accurate for cleaner size display. Refactoring is straightforward and well-executed. |
| src/core/config/descriptor_providers/schema_analyzer.cpp | 5/5 | Implemented determine_title method with validation. Properly enforces title requirement for leaf parameters while exempting objects. |
Sequence Diagram
sequenceDiagram
participant Main as main.cpp
participant SysVars as mce_sys_var
participant Registry as config_registry
participant Descriptor as config_descriptor
participant ParamDesc as parameter_descriptor
Note over Main,ParamDesc: Application Startup - Configuration Display
Main->>SysVars: safe_mce_sys().get_registry()
SysVars-->>Main: registry reference
Main->>Registry: get_config_descriptor()
Registry-->>Main: config_descriptor reference
Main->>Descriptor: get_parameter_map()
Descriptor-->>Main: parameter_map (all params)
loop For each parameter
Main->>ParamDesc: get_title()
ParamDesc-->>Main: display title
Main->>Registry: get_value_as_any(key)
Registry-->>Main: current value
Main->>ParamDesc: default_value()
ParamDesc-->>Main: default value
alt Special treatment exists
Main->>Main: Execute special handler
else Generic handling
alt Type is int64
Main->>ParamDesc: convert_int64_to_mapped_string(value)
ParamDesc-->>Main: string representation
Main->>Main: print_int64_config_element()
else Type is bool
Main->>Main: print_bool_config_element()
else Type is string
Main->>Main: print_string_config_element()
else Type is vector
Main->>Main: print_config_vector()
else Type is map
Main->>Main: print_config_map()
end
end
end
16 files reviewed, 1 comment
orshemesh-grok
force-pushed
the
redmine-4556521
branch
from
e8b1001 to
257f5b3
Compare
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors configuration parameter display from hardcoded per-parameter printing to automated registry scanning. The key changes:
- Config registry lifecycle: Registry now persists as a member of
mce_sys_varinstead of being created temporarily, enabling other code to access and iterate over all parameters - Automated display: Replaced ~300 lines of manual
VLOG_PARAM_*calls inmain.cppwith a generic loop that scans all registered parameters and prints non-default values - Helper infrastructure: Added
parameter_descriptor::get_title(),convert_int64_to_mapped_string(), and recursive printing functions for nested structures (vectors, maps) - Special cases preserved: Parameters requiring custom logic (log level, progress engine, etc.) are handled via a dispatch map
- Better error messages: Improved enum validation errors to list all valid options
The refactoring ensures new configuration parameters are automatically displayed without requiring manual code changes.
Confidence Score: 4/5
- This PR is mostly safe to merge with minor style issues to address
- The refactoring is well-structured and tested, but there's an unused variable in error handling and the existing concern about
convert_int64_to_mapped_string()returning misleading "(Invalid value)" text for unmapped values (already flagged in previous comments). These are not critical but should be addressed for code quality. - Pay attention to
src/core/config/config_registry.cpp(unused variable) andsrc/core/config/descriptors/parameter_descriptor.cpp(misleading display text already noted in previous comments)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/core/config/config_registry.cpp | 4/5 | Added catch block for runtime_error with unused variable - likely incomplete error handling |
| src/core/config/descriptors/parameter_descriptor.cpp | 3/5 | Implemented convert_int64_to_mapped_string with misleading "(Invalid value)" suffix for unmapped values, and improved error messages for invalid enum strings |
| src/core/main.cpp | 4/5 | Replaced manual parameter printing with automated registry scanning - significant refactoring with helper functions for vector/map display |
| src/core/util/sys_vars.h | 5/5 | Added config_registry member and getter, plus to_str_accurate helper - clean additions |
| src/core/util/sys_vars.cpp | 5/5 | Implemented to_str_accurate and changed to use member registry instead of local - straightforward refactoring |
Sequence Diagram
sequenceDiagram
participant App as Application Startup
participant MV as mce_sys_var
participant CR as config_registry
participant CD as config_descriptor
participant PD as parameter_descriptor
participant Main as main.cpp
App->>MV: Constructor
MV->>CR: Create m_registry
CR->>CD: Load descriptors from JSON schema
CD->>PD: Create parameter_descriptor for each param
PD-->>CD: Store title, type, constraints, mappings
CD-->>CR: Return config_descriptor
CR->>CR: Load values from sources (env, JSON, etc)
CR-->>MV: Registry ready
App->>Main: print_xlio_global_settings()
Main->>MV: get_registry()
MV-->>Main: Return config_registry reference
Main->>CR: get_config_descriptor()
CR-->>Main: Return config_descriptor
Main->>CD: get_parameter_map()
CD-->>Main: Return all parameters
loop For each parameter
alt Special treatment exists
Main->>Main: Execute special handler
else Generic handling
Main->>CR: get_value_as_any(key)
CR-->>Main: Return current value
Main->>PD: get_title(), default_value()
PD-->>Main: Return metadata
alt Type is int64 with mappings
Main->>PD: convert_int64_to_mapped_string(value)
PD-->>Main: Return string representation
end
Main->>Main: print_config_element()
end
end
16 files reviewed, 1 comment
orshemesh-grok
force-pushed
the
redmine-4556521
branch
from
257f5b3 to
d026f7e
Compare
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors the config parameter display logic to be data-driven and automatic, eliminating the need to manually add display code for each new parameter.
Key Changes:
- Lifecycle extension: The
config_registryobject is now a member variable ofmce_sys_var(instead of a local variable), ensuring it lives for the process lifetime and can be accessed at display time - Generic parameter iteration:
main.cppnow loops through all registered parameters viaconfig_descriptor::get_parameter_map()instead of hardcoding individual VLOG calls for each parameter - Title extraction: Parameter titles are now extracted from the JSON schema and stored in
parameter_descriptor, enabling automatic display with proper user-friendly names - String mapping support: Added
convert_int64_to_mapped_string()to display enum values as strings when mappings exist - Better error messages: Invalid enum values now show helpful error messages listing all valid options
Benefits:
- New config parameters are automatically displayed without code changes in
main.cpp - Reduced maintenance burden (removed ~300 lines of repetitive VLOG calls)
- Improved consistency in parameter display
- Better error messages for configuration validation
Issues Addressed by Previous Comments:
Most style issues from previous comments have been noted but the core refactoring logic is sound.
Confidence Score: 4/5
- This PR is safe to merge with minor style issues already flagged in previous comments
- The refactoring is well-structured and solves a real maintenance problem. The core logic change (registry lifetime extension and automatic parameter iteration) is sound. A unit test validates the improved error messaging. However, there are style issues (debug comments, unused variables, formatting) that have been flagged in previous comments but don't affect functionality. The "(Invalid value)" suffix in convert_int64_to_mapped_string could be misleading if parameters have partial string mappings, though current usage suggests all enum values have complete mappings.
- Pay close attention to
src/core/main.cpp(remove debug output),src/core/config/config_registry.cpp(fix indentation), andsrc/core/config/descriptors/parameter_descriptor.cpp(verify "(Invalid value)" behavior is acceptable)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/core/main.cpp | 4/5 | Replaced manual parameter logging with generic loop-based approach; uses registry to iterate all parameters automatically |
| src/core/config/config_registry.h | 5/5 | Added public accessor for config_descriptor and made get_value_as_any public to support parameter iteration |
| src/core/config/descriptors/parameter_descriptor.cpp | 4/5 | Implemented title accessors, improved error messages for invalid enum strings, added convert_int64_to_mapped_string with "(Invalid value)" suffix for unmapped integers |
| src/core/util/sys_vars.h | 5/5 | Changed config_registry from local variable to member variable for extended lifetime; added get_registry() accessor and to_str_accurate utility |
| src/core/util/sys_vars.cpp | 5/5 | Updated to use member registry instead of local instance; implemented to_str_accurate for size formatting without rounding |
Sequence Diagram
sequenceDiagram
participant Main as main.cpp
participant MceVar as mce_sys_var
participant Registry as config_registry
participant Descriptor as config_descriptor
participant ParamDesc as parameter_descriptor
Note over Main,ParamDesc: Initialization Phase
MceVar->>Registry: create (now member variable)
Registry->>Descriptor: initialize from JSON schema
Descriptor->>ParamDesc: create descriptors with titles
Note over Main,ParamDesc: Configuration Phase
MceVar->>Registry: apply_config_from_registry()
Registry->>Registry: load values from sources
Note over Main,ParamDesc: Display Phase (NEW)
Main->>MceVar: get_registry()
MceVar-->>Main: return registry reference
Main->>Registry: get_config_descriptor()
Registry-->>Main: return descriptor
Main->>Descriptor: get_parameter_map()
Descriptor-->>Main: return all parameters
loop For each parameter
Main->>Registry: get_value_as_any(key)
Registry-->>Main: current value
Main->>ParamDesc: get_title()
ParamDesc-->>Main: parameter title
Main->>ParamDesc: default_value()
ParamDesc-->>Main: default value
alt Has special treatment
Main->>Main: apply special logic
else Generic int64 parameter
Main->>ParamDesc: convert_int64_to_mapped_string(val)
ParamDesc-->>Main: mapped string or empty
alt String mapping found
Main->>Main: VLOG_STR_PARAM_STRING
else No string mapping
Main->>Main: VLOG_PARAM_STRING with to_str_accurate
end
else Other types
Main->>Main: print_bool/string/vector/map
end
end
16 files reviewed, no comments
tomerdbz
requested changes
Nov 5, 2025
Collaborator
tomerdbz
left a comment
tomerdbz
left a comment
There was a problem hiding this comment.
Great work Oren! Logic is sound, there's some additional matters of style, code modularity and optimization. We're almost there :)
orshemesh-grok
force-pushed
the
redmine-4556521
branch
from
d026f7e to
30ead2a
Compare
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Refactors config parameter printing to use registry iteration instead of hardcoded logging statements, ensuring all parameters (including new ones) are automatically displayed at startup.
Key changes:
- Created
config_printerclass that iterates overconfig_registryto print all parameters generically - Replaced ~330 lines of manual config printing in
main.cppwith call toconfig_printer::print_to_log() - Extended
config_registrylifetime from local variable tomce_sys_varmember to enable external access - Added parameter title support from JSON schema for better display names
- Enhanced error messages for invalid enum values using parameter titles
- Special handling preserved for parameters requiring non-standard display logic
Issues found:
- Compilation error in
config_printer.cpp:103- missing&config_printer::prefix for member function pointer
Confidence Score: 3/5
- Cannot merge - has compilation error that prevents build
- Well-designed refactoring with good architecture (registry pattern, clean separation of concerns), but contains a critical syntax error on line 103 of config_printer.cpp that will cause compilation failure. Once fixed, the changes are safe and achieve the stated goal.
- src/core/config_printer.cpp requires fix for compilation error on line 103
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/core/config_printer.cpp | 2/5 | New file that iterates over config registry to print parameters. Has compilation error on line 103 (missing &config_printer:: prefix). |
| src/core/main.cpp | 5/5 | Replaces ~330 lines of manual config printing with call to config_printer.print_to_log(), achieving the PR's goal. |
| src/core/util/sys_vars.cpp | 5/5 | Changes config_registry from local variable to member variable (m_registry), extending its lifetime. |
| src/core/config/config_registry.cpp | 5/5 | Adds catch for std::runtime_error to provide better error messages when string mappings validation fails. |
| src/core/config/descriptors/parameter_descriptor.cpp | 5/5 | Implements title support and convert_int64_to_mapped_string_or(). Enhanced error messages in convert_string_to_int64() using title. |
Sequence Diagram
sequenceDiagram
participant Main as main.cpp::print_xlio_global_settings()
participant Printer as config_printer
participant SysVar as mce_sys_var
participant Registry as config_registry
participant Descriptor as config_descriptor
participant ParamDesc as parameter_descriptor
Main->>Printer: config_printer(safe_mce_sys())
Main->>Printer: print_to_log()
Printer->>SysVar: get_registry()
SysVar-->>Printer: registry reference
Printer->>Registry: get_config_descriptor()
Registry-->>Printer: config_descriptor
Printer->>Descriptor: get_parameter_map()
Descriptor-->>Printer: parameter_map
loop For each parameter in parameter_map
Printer->>ParamDesc: get_title()
ParamDesc-->>Printer: title (optional)
alt Special parameter (log_level, rx_num_wre, etc.)
Printer->>Printer: call special handler (print_log_level, etc.)
else Generic parameter
Printer->>Registry: get_value_as_any(key)
Registry-->>Printer: current value
Printer->>ParamDesc: default_value()
ParamDesc-->>Printer: default value
alt Integer with string mapping
Printer->>ParamDesc: has_string_mappings()
ParamDesc-->>Printer: true
Printer->>ParamDesc: convert_int64_to_mapped_string_or(val, default)
ParamDesc-->>Printer: mapped string or default
end
Printer->>Printer: print_config_element(key, descriptor, value, default)
Printer->>Printer: Log via VLOG_NUM_PARAM_DETAILS or VLOG_STR_PARAM_DETAILS
end
end
23 files reviewed, 1 comment
orshemesh-grok
force-pushed
the
redmine-4556521
branch
from
30ead2a to
38a58ee
Compare
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors the configuration printing mechanism from a manual, per-parameter approach to an automatic registry-scanning approach. Previously, each config variable required explicit code in main.cpp to display its value at startup. Now, the config registry is made persistent throughout the process lifetime and a new config_printer class iterates through all registered parameters automatically.
Key Changes:
config_registrymoved from local variable tomce_sys_varmember, extending its lifetime beyond initialization- New
config_printerclass encapsulates logic for displaying config values with proper formatting - Removed 300+ lines of manual
VLOG_PARAM_*macro calls inmain.cpp - Enhanced
parameter_descriptorwith title field and safer string-to-int mapping conversion - Added public accessors to
config_registryandconfig_descriptorfor external iteration
Benefits:
- New config parameters are automatically displayed without code changes
- Reduces maintenance burden and eliminates risk of forgetting to add display code
- Centralizes config printing logic in dedicated class
Confidence Score: 2/5
- This PR contains a critical compilation error that will prevent the code from building
- The code has a syntax error on line 103 of
config_printer.cppwhere a member function pointer is missing the class qualifier (&config_printer::). This will cause immediate compilation failure. While the refactoring approach is sound and most code is well-implemented, the presence of a build-blocking error requires fixing before merge - Pay close attention to
src/core/config_printer.cpp(syntax error on line 103)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/core/config_printer.cpp | 1/5 | New file implementing automatic config printing - contains critical syntax error on line 103 preventing compilation |
| src/core/config_printer.h | 4/5 | New header for config_printer class - clean implementation with minor conditional compilation issue |
| src/core/main.cpp | 5/5 | Refactored to use config_printer - removed 300+ lines of manual config printing code |
| src/core/config/descriptors/parameter_descriptor.cpp | 5/5 | Added title field and convert_int64_to_mapped_string_or method - improved error messages for invalid string mappings |
Sequence Diagram
sequenceDiagram
participant App as Application Startup
participant Main as main.cpp
participant MceVar as mce_sys_var
participant Registry as config_registry
participant Printer as config_printer
participant ParamDesc as parameter_descriptor
App->>Main: print_xlio_global_settings()
Main->>MceVar: get safe_mce_sys()
Main->>Printer: config_printer(safe_mce_sys())
Printer->>MceVar: get_registry()
MceVar-->>Printer: return m_registry reference
Printer->>Registry: get_config_descriptor()
Registry-->>Printer: return config_descriptor
Printer->>Printer: get_parameter_map()
loop For each parameter
Printer->>Printer: Check special_treatments map
alt Has special handling
Printer->>Printer: Call specialized print function
Printer->>MceVar: Access specific sys_var field
Printer->>Printer: Log with custom logic
else Generic handling
Printer->>Registry: get_value_as_any(key)
Registry-->>Printer: return value
Printer->>ParamDesc: get string mappings (if applicable)
ParamDesc-->>Printer: convert_int64_to_mapped_string_or()
Printer->>Printer: print_config_element()
alt Non-default value
Printer->>Main: vlog_printf(VLOG_INFO)
else Default value
Printer->>Main: vlog_printf(VLOG_DETAILS)
end
end
end
23 files reviewed, 2 comments
orshemesh-grok
force-pushed
the
redmine-4556521
branch
from
38a58ee to
352d8b9
Compare
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors the configuration variable printing system to be registry-driven rather than hardcoded, ensuring all config variables are automatically displayed at startup without requiring explicit code for each parameter.
Key Changes
- New
config_printerclass: Centralizes config printing logic by iterating through the registry instead of hardcoding each parameter - Registry lifetime extension: Changed
config_registryfrom a local variable to a member ofmce_sys_varto allow access throughout application lifetime - Enhanced parameter metadata: Added title field and
convert_int64_to_mapped_string_or()toparameter_descriptorfor better display formatting - Significant code reduction: Replaced ~300 lines of repetitive printing code in
main.cppwith a single loop-based implementation - Special case handling: Maintains custom formatting for parameters that need non-standard display (e.g., log level, progress engine interval)
Benefits
- Automatic coverage: New config parameters are shown automatically without needing manual print statements
- Maintainability: Single source of truth for parameter printing logic
- Better error messages: Enhanced error reporting during config initialization with runtime_error handling
Confidence Score: 5/5
- This PR is safe to merge - it's a well-designed refactoring with comprehensive testing
- The changes are well-architected with proper separation of concerns, backward compatibility is maintained through special case handlers, and the new
convert_int64_to_mapped_string_or()function safely handles unmapped values. All style issues from previous comments have been addressed, and the implementation includes unit tests validating the new functionality. - No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/core/config_printer.cpp | 5/5 | New file implementing config parameter printing by iterating through registry instead of hardcoded parameters |
| src/core/main.cpp | 5/5 | Replaced 300+ lines of hardcoded config printing with single config_printer instance call |
| src/core/config/config_registry.h | 5/5 | Made get_value_as_any() public and added get_config_descriptor() accessor for registry iteration |
| src/core/config/descriptors/parameter_descriptor.cpp | 5/5 | Implemented title getter/setter and safe int-to-string conversion with better error messages |
| src/core/util/sys_vars.h | 5/5 | Added config_registry member and accessor to make registry available throughout application lifetime |
Sequence Diagram
sequenceDiagram
participant Main as main.cpp
participant Printer as config_printer
participant Registry as config_registry
participant Descriptor as config_descriptor
participant ParamDesc as parameter_descriptor
Main->>Printer: config_printer(mce_sys_var)
Main->>Printer: print_to_log()
Printer->>Registry: get_config_descriptor()
Registry-->>Printer: config_descriptor
Printer->>Descriptor: get_parameter_map()
Descriptor-->>Printer: parameter_map
loop For each parameter
Printer->>Printer: Check special_treatments map
alt Has special treatment
Printer->>Printer: Call specialized print function
else Generic handling
Printer->>Registry: get_value_as_any(key)
Registry-->>Printer: current value
Printer->>ParamDesc: get_title()
ParamDesc-->>Printer: title
Printer->>ParamDesc: default_value()
ParamDesc-->>Printer: default value
alt Has string mappings
Printer->>ParamDesc: convert_int64_to_mapped_string_or()
ParamDesc-->>Printer: mapped string
end
Printer->>Printer: print_config_element()
end
end
23 files reviewed, no comments
orshemesh-grok
force-pushed
the
redmine-4556521
branch
from
352d8b9 to
e9fb0b7
Compare
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Refactored configuration printing from hardcoded per-parameter logging to automated registry scanning, ensuring all config variables are automatically displayed at startup without manual code updates.
Key Changes
- New
config_printerclass (src/core/config_printer.cpp): Extracted ~330 lines of printing logic from main.cpp into a dedicated class that iterates over the configuration registry - Registry lifetime extension:
config_registrymoved from stack tomce_sys_varmember (m_registry) to persist throughout process lifetime - Enhanced parameter_descriptor: Added
titlefield,has_string_mappings()check, andconvert_int64_to_mapped_string_or()for safe display conversion - Improved error messages: Enhanced string mapping validation to show valid values when invalid input provided
- Comprehensive test coverage: Added unit tests for new string conversion methods
Implementation Details
The refactoring maintains backward compatibility by preserving special handling for parameters with non-standard display logic (log_level, rx_num_wre, progress_engine_interval, etc.) via function pointer dispatch map, while generic parameters use type-based automatic printing.
Confidence Score: 5/5
- This PR is safe to merge - well-architected refactoring with comprehensive test coverage and no breaking changes
- The implementation is clean, well-tested, and solves the stated problem elegantly. The code properly handles all previous comments from reviewers (removed debug lines, fixed syntax errors). The registry-based approach is more maintainable than hardcoded parameter printing, and special cases are preserved correctly. No logical errors or security concerns identified.
- No files require special attention - all changes are well-implemented and tested
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/core/config_printer.cpp | 5/5 | New file extracting config printing logic from main.cpp - implements automated parameter scanning via registry iteration |
| src/core/config_printer.h | 5/5 | Header for config_printer class with print methods for different parameter types and special cases |
| src/core/main.cpp | 5/5 | Replaced ~330 lines of manual config printing with 2-line call to config_printer, significant simplification |
| src/core/util/sys_vars.cpp | 5/5 | Changed to use member m_registry instead of creating temporary config_registry instance |
| src/core/util/sys_vars.h | 5/5 | Added m_registry member and get_registry() accessor to enable external access to configuration registry |
| src/core/config/config_registry.h | 5/5 | Added get_config_descriptor() accessor method to expose configuration metadata for iteration |
| src/core/config/descriptors/parameter_descriptor.cpp | 5/5 | Added title field, convert_int64_to_mapped_string_or() method for display, and improved error messages for invalid string mappings |
| src/core/config/descriptors/parameter_descriptor.h | 5/5 | Added title field accessors, has_string_mappings() check, and convert_int64_to_mapped_string_or() for safe string conversion |
Sequence Diagram
sequenceDiagram
participant Main as main.cpp
participant SysVars as mce_sys_var
participant Registry as config_registry
participant Printer as config_printer
participant Descriptor as config_descriptor
participant ParamDesc as parameter_descriptor
Note over Main: Startup: print_xlio_global_settings()
Main->>SysVars: safe_mce_sys()
Main->>Printer: new config_printer(mce_sys_var)
Main->>Printer: print_to_log()
Printer->>SysVars: get_registry()
SysVars-->>Printer: m_registry reference
Printer->>Registry: get_config_descriptor()
Registry-->>Printer: config_descriptor
Printer->>Descriptor: get_parameter_map()
Descriptor-->>Printer: parameter_map (all params)
loop For each parameter
Printer->>ParamDesc: get_title()
ParamDesc-->>Printer: title string
alt Special parameter (log_level, rx_num_wre, etc)
Printer->>Printer: call special handler
Printer->>SysVars: access field directly
else Generic parameter
Printer->>Registry: get_value_as_any(key)
Registry-->>Printer: current value
Printer->>ParamDesc: default_value()
ParamDesc-->>Printer: default value
alt Has string mapping
Printer->>ParamDesc: has_string_mappings()
ParamDesc-->>Printer: true
Printer->>ParamDesc: convert_int64_to_mapped_string_or()
ParamDesc-->>Printer: mapped string or default
end
Printer->>Printer: print_config_element()
Note over Printer: Log at INFO if non-default<br/>Log at DETAILS if default
end
end
23 files reviewed, no comments
tomerdbz
reviewed
Nov 9, 2025
Collaborator
tomerdbz
left a comment
tomerdbz
left a comment
There was a problem hiding this comment.
Final considerations before merge
Collaborator
|
@dpressle - why CI doesn't run here? |
orshemesh-grok
force-pushed
the
redmine-4556521
branch
from
e9fb0b7 to
04a116f
Compare
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors config variable display from hardcoded per-variable printing to automated registry-based scanning. This ensures new config variables are automatically shown at startup without manual code additions.
Key Changes:
- Replaced ~300 lines of manual
VLOG_PARAM_*calls inmain.cppwith a loop over the config registry - Created new
config_printerclass that iterates through all registered parameters and displays non-default values at INFO level, all values at DETAILS level - Extended
config_registrylifetime by storing it as anoptionalmember inmce_sys_var(destroyed after config printing to free memory) - Added
convert_int64_to_mapped_string_or()method for safe string conversion of enum-like int64 values - Added title support to parameters for better display names
- Improved error messages for invalid enum values by listing valid options
- Preserved special handling for complex parameters (log_level, rx_num_wre, progress_engine_interval, nginx/envoy workers)
Benefits:
- Eliminates risk of forgetting to add display code for new config variables
- Reduces code duplication and maintenance burden
- Consistent formatting across all config parameter displays
- Better error messages with context
Confidence Score: 4/5
- This PR is safe to merge with minor risk - the refactoring is well-architected and addresses previous review comments, though the large scope means thorough testing is recommended.
- Score of 4 reflects solid implementation with proper error handling, test coverage for new methods, and careful preservation of special-case behavior. Previous review comments about unsafe string conversion and missing member function prefixes have been addressed. The main concern is the large refactoring scope (replacing manual logging of 100+ config parameters), which increases risk despite the clean implementation.
- Pay close attention to
src/core/config_printer.cpp- ensure all special-case parameters are handled correctly and that the automated display matches the old manual approach for all parameter types.
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/core/config_printer.cpp | 4/5 | New file implementing automated config printing via registry scanning. Implementation looks solid with proper type handling and special cases for complex parameters. |
| src/core/main.cpp | 5/5 | Replaced 300+ lines of manual config printing with automated registry-based approach. Registry properly destroyed after use to free memory. |
| src/core/config/config_registry.h | 5/5 | Simplified template methods and made get_value_as_any() public. Added get_config_descriptor() accessor for registry scanning. |
| src/core/config/descriptors/parameter_descriptor.cpp | 4/5 | Added convert_int64_to_mapped_string_or() for safe string conversion, improved error messages for invalid enum values, added title support. |
| src/core/util/sys_vars.h | 5/5 | Registry stored as optional member, with get_registry() and destroy_registry() methods. Placed at end to avoid cache-line disruption. |
Sequence Diagram
sequenceDiagram
participant M as main.cpp
participant CP as config_printer
participant SV as mce_sys_var
participant CR as config_registry
participant CD as config_descriptor
participant PD as parameter_descriptor
M->>M: print_xlio_global_settings()
M->>CP: config_printer(safe_mce_sys())
M->>CP: print_to_log()
CP->>SV: get_registry()
SV-->>CP: optional<config_registry>
CP->>CR: get_config_descriptor()
CR-->>CP: config_descriptor&
CP->>CD: get_parameter_map()
CD-->>CP: parameter_map
loop For each parameter in map
CP->>PD: get_title()
PD-->>CP: optional<string>
alt Special handling needed
CP->>CP: print_log_level/print_rx_num_wre/etc()
else Generic handling
CP->>CR: get_value_as_any(key)
CR-->>CP: any value
CP->>PD: get_default_value()
PD-->>CP: any default
alt int64 with string mapping
CP->>PD: has_string_mappings()
PD-->>CP: true
CP->>PD: convert_int64_to_mapped_string_or()
PD-->>CP: string representation
end
CP->>CP: print_config_element()
CP->>CP: vlog_printf(VLOG_INFO/VLOG_DETAILS)
end
end
M->>SV: destroy_registry()
SV->>SV: m_registry = nullopt
24 files reviewed, no comments
Author
|
bot:retest |
Author
|
bot:retest |
1 similar comment
Author
|
bot:retest |
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR adds comprehensive testing for the new automated config display functionality. A new test file config.cc was created to verify that config parameters are displayed correctly at startup.
Key changes:
- New test base class
outputwith helpers to execute commands and capture XLIO output - Five test cases covering different scenarios of acceleration control rules display (2, 4, 5, and 6 rules)
- Tests verify formatting of various config types: numbers, strings, booleans, enums, arrays
- Tests validate the "element(s) not shown" message appears correctly for large arrays
Test coverage:
- Validates various config parameter display formats (4K notation, "0 (Disabled)" special case, enum-to-string mappings)
- Tests array element truncation behavior for large config arrays
- Ensures config sources are shown properly
Confidence Score: 4/5
- This PR is safe to merge with minor typo corrections needed
- The test file is well-structured with comprehensive coverage of config display functionality. Only two minor typos were found that need fixing. The logic and test structure are sound.
- tests/gtest/output/config.cc requires typo fixes on lines 71 and 187 before merging
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| tests/gtest/output/config.cc | 4/5 | New test file for config display. Has two typos: "sou rces" spacing on line 71 and incorrect comment on line 187 saying "4 rules" instead of "5 rules". |
Sequence Diagram
sequenceDiagram
participant Test as Test Case
participant Helper as check_file()
participant Shell as popen/ls
participant XLIO as XLIO Library
participant ConfigReg as Config Registry
Test->>Helper: check_file("config-sample.json", expected[], unexpected[])
Helper->>Shell: Execute "XLIO_USE_NEW_CONFIG=1 XLIO_CONFIG_FILE=... ls 2>&1"
Shell->>XLIO: LD_PRELOAD triggers XLIO initialization
XLIO->>ConfigReg: Load and display config parameters
ConfigReg-->>XLIO: Config output (log lines)
XLIO-->>Shell: stderr/stdout output
Shell-->>Helper: Captured text output
loop For each expected string
Helper->>Helper: ASSERT_TRUE(text.find(expected) != npos)
end
loop For each unexpected string
Helper->>Helper: ASSERT_TRUE(text.find(unexpected) == npos)
end
Helper-->>Test: Test passes/fails
1 file reviewed, 2 comments
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR adds comprehensive test coverage for the new config display feature that shows all non-default configuration variables programmatically at startup.
Key Changes:
- Added new test file
tests/gtest/output/config.ccwith 4 test cases validating config output formatting - Tests verify various config types: simple values, booleans, strings, enums, arrays, and special formatting (e.g., 4096 shown as "4K")
- Tests validate the "element(s) not shown" message appears correctly for acceleration control rules arrays with >4 elements
- Modified
gtest.shto passXLIO_WORKSPACEenvironment variable needed by the new tests - Tests use
popen()to capture XLIO startup output and validate expected strings appear
Test Structure:
The test class executes ls with LD_PRELOAD and different config JSON files, capturing XLIO's startup output to verify config parameters are displayed correctly. Four test cases cover 6-rule, 2-rule, 4-rule, and 5-rule scenarios for acceleration control arrays.
Confidence Score: 4/5
- This PR is safe to merge with minor syntax issues that should be fixed
- The changes are test-only additions with no production code changes in these files. Two typos in comments should be corrected (lines 73 and 211), but these don't affect test execution. The test logic is sound and provides good coverage for the config display feature.
- tests/gtest/output/config.cc needs typo corrections in comments on lines 73 and 211
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| contrib/jenkins_tests/gtest.sh | 5/5 | Added XLIO_WORKSPACE environment variable to test command to support new config output tests |
| tests/gtest/output/config.cc | 4/5 | New test file validating config output display; contains typos in comment on lines 73 and 211 |
Sequence Diagram
sequenceDiagram
participant JenkinsTest as Jenkins Test Script
participant GTest as GTest Framework
participant OutputTest as output::check_file()
participant Shell as Shell (popen)
participant XLIO as XLIO Library (LD_PRELOAD)
participant ConfigRegistry as Config Registry
JenkinsTest->>JenkinsTest: Set XLIO_WORKSPACE env var
JenkinsTest->>GTest: Run gtest with LD_PRELOAD
GTest->>OutputTest: TEST_F(output, config_show_sample_values)
OutputTest->>OutputTest: Build path to config JSON file
OutputTest->>Shell: popen("XLIO_CONFIG_FILE=... ls 2>&1")
Shell->>XLIO: Load library via LD_PRELOAD
XLIO->>ConfigRegistry: Read config from JSON file
ConfigRegistry->>XLIO: Parse and validate config
XLIO->>XLIO: Display config values at startup
XLIO-->>Shell: Output logs with config values
Shell-->>OutputTest: Return captured text output
OutputTest->>OutputTest: Check for expected strings
OutputTest->>OutputTest: Check for unexpected strings
OutputTest-->>GTest: ASSERT_TRUE results
GTest-->>JenkinsTest: Test pass/fail
2 files reviewed, 2 comments
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Adds comprehensive test suite for programmatic config variable display at startup. The tests validate that config parameters are shown correctly through JSON files, including handling of arrays, nested objects, and truncation messages.
Key changes:
- New test file
tests/gtest/output/config.ccwith 4 test cases covering different scenarios - Tests validate config display with 2, 4, 5, and 6 acceleration rules to verify truncation logic
- Uses
system()command to spawn process with LD_PRELOAD and captures output to file - Comprehensive assertions check for expected strings in output including numeric, boolean, string, and enum values
Confidence Score: 4/5
- This PR is safe to merge with minor cleanup recommended
- Score reflects well-structured test implementation with comprehensive coverage. The test file is newly added and validates the programmatic config display feature. Previous syntax issues (typos in comments) have been fixed in the latest commit. Only minor style issue remains: commented-out code that should be removed for cleaner codebase.
- No files require special attention - only minor style cleanup needed in tests/gtest/output/config.cc
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| tests/gtest/output/config.cc | 4/5 | New test file validating programmatic config output display - minor cleanup needed for commented-out code |
Sequence Diagram
sequenceDiagram
participant Test as Test Runner
participant TestCase as config_show_sample_values
participant System as system()
participant XLIO as XLIO Library (LD_PRELOAD)
participant ConfigPrinter as config_printer
participant Registry as config_registry
participant File as output.txt
Test->>TestCase: Execute TEST_F
TestCase->>TestCase: SetUp() - read XLIO_WORKSPACE
TestCase->>TestCase: check_file("config-sample.json")
TestCase->>System: system("XLIO_USE_NEW_CONFIG=1 XLIO_CONFIG_FILE=... ls > output.txt 2>&1")
System->>XLIO: Launch process with LD_PRELOAD
XLIO->>Registry: Load config from JSON file
Registry->>Registry: Parse and store parameters
XLIO->>ConfigPrinter: print_to_log()
ConfigPrinter->>Registry: get_registry().get_config_descriptor()
Registry-->>ConfigPrinter: parameter_map
loop For each parameter
ConfigPrinter->>ConfigPrinter: Check special_treatments map
alt Special treatment exists
ConfigPrinter->>ConfigPrinter: Call special handler
else Generic handling
ConfigPrinter->>Registry: get_value_as_any(key)
Registry-->>ConfigPrinter: Current value
ConfigPrinter->>ConfigPrinter: print_config_element()
end
ConfigPrinter->>File: Write formatted output
end
XLIO->>Registry: destroy_registry()
System-->>TestCase: Exit code 0
TestCase->>File: Read output.txt
File-->>TestCase: Output text
loop For each expected string
TestCase->>TestCase: ASSERT_TRUE(text.find(expected))
end
loop For each unexpected string
TestCase->>TestCase: ASSERT_TRUE(text.find(unexpected) == npos)
end
TestCase-->>Test: Test result
1 file reviewed, 1 comment
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors config variable display from hardcoded lines to registry scanning, automatically showing all config values at startup.
Key Changes:
- Adds
titlefield toparameter_descriptorandschema_analyzerfor human-readable parameter names - Introduces
convert_int64_to_mapped_string_or()method with fallback for displaying unmapped int64 values - Creates new
config_printerclass that iterates through the config registry instead of manual printing - Adds comprehensive test suite in
tests/gtest/output/config.ccto validate formatted output - Registry now persists during config display and is explicitly destroyed after use (
destroy_registry()called in main.cpp:886)
Benefits:
- New config parameters are automatically displayed without code changes
- Improved maintainability - no need to update display code for each new parameter
- Better error messages for invalid enum values (includes list of valid options)
- Consistent formatting across all config parameters
Architecture:
The refactoring moves from imperative per-variable logging to declarative registry-based iteration, maintaining backward compatibility while improving extensibility.
Confidence Score: 4/5
- Safe to merge with minor considerations - solid refactoring with comprehensive tests
- Score reflects well-structured refactoring with good test coverage. The changes are mostly additive (new methods, new test file) with minimal modifications to existing logic. Previous review comments have been addressed. The main concern is the registry lifecycle change (now persists longer) but this is intentional and properly managed with explicit cleanup.
- No files require special attention - all changes follow clean design patterns
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/core/config/descriptors/parameter_descriptor.cpp | 4/5 | Implements title handling and convert_int64_to_mapped_string_or with fallback support - improved error messages for invalid values |
| tests/gtest/output/config.cc | 5/5 | New comprehensive test suite for config display - validates formatted output with various rule counts |
Sequence Diagram
sequenceDiagram
participant Main as main.cpp
participant Printer as config_printer
participant Registry as config_registry
participant Descriptor as config_descriptor
participant Param as parameter_descriptor
Main->>Printer: create config_printer(mce_sys_var)
Main->>Printer: print_to_log()
Printer->>Registry: get_registry()
Printer->>Registry: get_config_descriptor()
Registry-->>Printer: return config_descriptor
Printer->>Descriptor: get_parameter_map()
Descriptor-->>Printer: return parameter_map
loop For each parameter in map
Printer->>Printer: Check if special_treatment needed
alt Special treatment exists
Printer->>Printer: call special handler (e.g., print_log_level)
else Generic handling
Printer->>Registry: get_value_as_any(key)
Registry-->>Printer: return value
Printer->>Param: get_title()
Param-->>Printer: return title
Printer->>Param: has_string_mappings()
Param-->>Printer: return bool
alt Has string mappings
Printer->>Param: convert_int64_to_mapped_string_or()
Param-->>Printer: return string representation
end
Printer->>Printer: print_config_element()
Printer->>Printer: vlog_printf (VLOG_INFO or VLOG_DETAILS)
end
end
Main->>Registry: destroy_registry()
Registry-->>Main: free memory
5 files reviewed, no comments
Author
|
bot:retest |
Greptile OverviewGreptile SummaryThis PR refactors configuration variable display at startup to use a generic registry-based approach instead of hardcoded printing for each variable. The change eliminates ~300 lines of repetitive code in Key changes:
Architecture improvements:
Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Main as main.cpp
participant SysVars as mce_sys_var
participant Registry as config_registry
participant Printer as config_printer
participant Descriptor as config_descriptor
participant ParamDesc as parameter_descriptor
Note over Main,ParamDesc: Initialization Phase
Main->>SysVars: apply_config_from_registry()
SysVars->>Registry: construct config_registry()
Registry->>Descriptor: load schema & parameters
Descriptor->>ParamDesc: create parameter_descriptor with title
SysVars->>SysVars: store registry in m_registry (optional)
Note over Main,ParamDesc: Config Display Phase
Main->>Main: print_xlio_global_settings()
Main->>Printer: construct config_printer(mce_sys_var)
Main->>Printer: print_to_log()
Printer->>SysVars: get_registry().value()
SysVars-->>Printer: return config_registry&
Printer->>Registry: get_config_descriptor()
Registry-->>Printer: return config_descriptor&
Printer->>Descriptor: get_parameter_map()
Descriptor-->>Printer: return parameter_map
loop For each parameter
Printer->>ParamDesc: get_title()
ParamDesc-->>Printer: return title
alt Special treatment needed
Printer->>Printer: call special handler
else Generic handling
Printer->>Registry: get_value_as_any(key)
Registry-->>Printer: return current value
Printer->>ParamDesc: get default_value()
ParamDesc-->>Printer: return default value
Printer->>ParamDesc: has_string_mappings()
ParamDesc-->>Printer: return bool
alt Has string mappings
Printer->>ParamDesc: convert_int64_to_mapped_string_or()
ParamDesc-->>Printer: return string representation
end
Printer->>Printer: log value if non-default
end
end
Note over Main,ParamDesc: Cleanup Phase
Main->>SysVars: destroy_registry()
SysVars->>SysVars: m_registry = nullopt (frees memory)
|
orshemesh-grok
force-pushed
the
redmine-4556521
branch
from
4515ab4 to
ea6741f
Compare
- Exsposed the config registry for use by other parts of the code - Refactored the display of non-default configurations to scan the config registry Signed-off-by: Oren Shemesh <[email protected]>
orshemesh-grok
force-pushed
the
redmine-4556521
branch
from
e5e0f1c to
cb85060
Compare
Author
|
bot:retest |
tomerdbz
approved these changes
Nov 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Show config variables at startup by scanning the registry and not by a specific line of code for each config variable.
This means that new config variables are shown automatically, without the need to remember to add code that shows them in main.cc
What
Show config variables at startup by scanning the registry and not by a specific line of code showing each config variable.
Why ?
https://redmine.mellanox.com/issues/4556521
How ?
Change type
What kind of change does this PR introduce?
Check list