Options variables converted to a structure#469
Options variables converted to a structure#469gsnw-sebast wants to merge 2 commits intoschweikert:developfrom
Conversation
|
coverage: 87.937% (-0.06%) from 88.0% — gsnw:flags-variable-to-struct into schweikert:develop |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the configuration system by consolidating global option variables into a centralized Options structure. It includes the definition of the struct, an initialization function for default values, and updates to all references throughout the source code. The review feedback highlights a potential breaking change in debug log formats and suggests further encapsulating remaining debug flags within the new structure for consistency.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the configuration system by grouping global option variables into Options and DebugOptions structures, introducing initialization functions, and updating all references throughout the codebase. The review feedback identifies an opportunity to correct inconsistent indentation in the new initialization logic, suggests refining debug output strings to hide internal struct prefixes for better readability, and recommends including flags.h within fping.h to ensure the header is self-contained.
| if (dbg_opt.trace_on) { | ||
| fprintf(stderr, "%s:\n opt.count: %u, opt.retry: %u, opt.interval: %.0f ms\n", | ||
| prog, opt.count, opt.retry, opt.interval / 1e6); | ||
| fprintf(stderr, " opt.perhost_interval: %.0f ms, opt.timeout: %.0f\n", | ||
| opt.perhost_interval / 1e6, opt.timeout / 1e6); | ||
| fprintf(stderr, " opt.seqmap_timeout: %.0f\n", opt.seqmap_timeout / 1e6); | ||
| fprintf(stderr, " opt.ping_data_size = %u, trials = %u\n", | ||
| opt.ping_data_size, trials); | ||
|
|
||
| if (opt.verbose_on) | ||
| fprintf(stderr, " opt.verbose_on set\n"); | ||
| if (opt.multif_on) | ||
| fprintf(stderr, " opt.multif_on set\n"); | ||
| if (opt.name_on) | ||
| fprintf(stderr, " opt.name_on set\n"); | ||
| if (opt.addr_on) | ||
| fprintf(stderr, " opt.addr_on set\n"); | ||
| if (opt.stats_on) | ||
| fprintf(stderr, " opt.stats_on set\n"); | ||
| if (opt.unreachable_on) | ||
| fprintf(stderr, " opt.unreachable_on set\n"); | ||
| if (opt.alive_on) | ||
| fprintf(stderr, " opt.alive_on set\n"); | ||
| if (opt.elapsed_on) | ||
| fprintf(stderr, " opt.elapsed_on set\n"); | ||
| if (opt.version_on) | ||
| fprintf(stderr, " opt.version_on set\n"); | ||
| if (opt.count_on) | ||
| fprintf(stderr, " opt.count_on set\n"); | ||
| if (opt.loop_on) | ||
| fprintf(stderr, " opt.loop_on set\n"); | ||
| if (opt.backoff_on) | ||
| fprintf(stderr, " opt.backoff_on set\n"); | ||
| if (opt.per_recv_on) | ||
| fprintf(stderr, " opt.per_recv_on set\n"); | ||
| if (opt.report_all_rtts_on) | ||
| fprintf(stderr, " opt.report_all_rtts_on set\n"); | ||
| if (dbg_opt.randomly_lose_on) | ||
| fprintf(stderr, " dbg_opt.randomly_lose_on set\n"); | ||
| if (dbg_opt.print_per_system_on) | ||
| fprintf(stderr, " dbg_opt.print_per_system_on set\n"); | ||
| if (opt.outage_on) | ||
| fprintf(stderr, " opt.outage_on set\n"); | ||
| if (opt.print_netdata_on) | ||
| fprintf(stderr, " opt.print_netdata_on set\n"); | ||
| if (opt.print_json_on) | ||
| fprintf(stderr, " opt.print_json_on set\n"); | ||
| } |
There was a problem hiding this comment.
The debug messages in this block now print variable names with their struct prefixes (e.g., opt.count, dbg_opt.randomly_lose_on). This exposes implementation details in the debug output, making it less clean and harder to read.
Please consider removing the opt. and dbg_opt. prefixes from the format strings to only print the option names.
For example:
// Before
fprintf(stderr, "%s:\n opt.count: %u, opt.retry: %u, opt.interval: %.0f ms\n",
prog, opt.count, opt.retry, opt.interval / 1e6);
if (dbg_opt.randomly_lose_on)
fprintf(stderr, " dbg_opt.randomly_lose_on set\n");
// After
fprintf(stderr, "%s:\n count: %u, retry: %u, interval: %.0f ms\n",
prog, opt.count, opt.retry, opt.interval / 1e6);
if (dbg_opt.randomly_lose_on)
fprintf(stderr, " randomly_lose_on set\n");This should be applied to all the fprintf calls in this debug block.
There was a problem hiding this comment.
In my opinion, the current issue is a good fit.
gsnw-sebast
force-pushed
the
flags-variable-to-struct
branch
from
e0217dc to
3f3849a
Compare
gsnw-sebast
force-pushed
the
flags-variable-to-struct
branch
from
3f3849a to
cd00560
Compare
2 participants
I've wrapped the option variables in a structure.
This improves the following: