Replace getopt_long with popl.hpp for CLI option parsing#302
Replace getopt_long with popl.hpp for CLI option parsing#302
Conversation
- Vendor popl.hpp as a single-header dependency - Replace manual parsing logic with popl::OptionParser - Preserve all existing CLI behavior and defaults - Maintain repeatable options, --pid and -- prog modes - All 14 existing tests pass (ctest 14/14)
|
Hi @graeme-a-stewart , this PR is ready for review. Please let me know if any changes are required. |
|
Thank you @iareARiES. I have been in a workshop this week, but next week I will review this. |
|
Just a quick observation - you have committed some files that should not be in the repository. Please remove them. Thanks. |
|
@graeme-a-stewart Thanks for pointing that out. |
graeme-a-stewart
left a comment
graeme-a-stewart
left a comment
There was a problem hiding this comment.
I pointed up a few places where the code can be improved, but broadly looking good @iareARiES.
package/src/prmon.cpp
Outdated
| // See https://github.com/HSF/prmon | ||
|
|
||
| #include <getopt.h> | ||
| #include "popl.hpp" |
There was a problem hiding this comment.
Small point: Just in terms of header ordering, as popl.hpp is now a local header, if should be moved to third group of headers, which are the other local headers (and ordered alphabetically).
package/src/prmon.cpp
Outdated
| "o", "log-filename", "Filename for logging", "prmon.log"); | ||
| auto opt_interval = op.add<popl::Value<unsigned int>>( | ||
| "i", "interval", "Seconds between samples", 30); | ||
| auto opt_suppress = op.add<popl::Switch>( |
There was a problem hiding this comment.
To be more specific, I would call the POPL variable here opt_suppress_hw_info.
package/src/prmon.cpp
Outdated
| "prmon is a process monitor program that records runtime data\n" | ||
| "from a process and its children, writing time stamped values\n" | ||
| "for resource consumption into a logfile and a JSON summary\n" | ||
| "format when the process exits."); |
There was a problem hiding this comment.
As POPL adds a semicolon after this descriptive text, it looks nicer if you remove the final full stop.
package/src/prmon.cpp
Outdated
| "format when the process exits."); | ||
|
|
||
| auto opt_pid = op.add<popl::Value<int>>( | ||
| "p", "pid", "Monitored process ID", -1); |
There was a problem hiding this comment.
Remove the default -1 value - it looks odd when POPL prints this meaningless default. You use opt_pid->is_set(); below anyway and can adapt the logic to adopt an internal default for pid.
package/src/prmon.cpp
Outdated
| break; | ||
| } | ||
| // Extract values from parsed options | ||
| pid_t pid = opt_pid->value(); |
There was a problem hiding this comment.
Something like:
pid_t pid = -1;
bool got_pid = opt_pid->is_set();
if (got_pid) {
pid = opt_pid->value();
}Is better and avoids exposing a meaningless default to the user.
No functional changes were made.
|
@graeme-a-stewart Thanks for the Review
No functional changes were made. |
2 participants
Addresses #243
The existing argument parsing in
main()usedgetopt_long, which required maintaining three separate places in sync for every option — thestatic struct optionarray, the switch-case block, and the manualif (do_help)help text. Adding or changing any option meant updating all three. The help text was entirely hand-written, so it could silently go out of sync with the actual options.What I changed
Only
package/src/prmon.cppwas modified:#include <getopt.h>for#include "popl.hpp"long_options[]array, thegetopt_longwhile loop, and the ~60 line manual help blockpopl::OptionParserregistering all 12 options in one place--separator scan is preserved —argvis scanned for--beforeop.parse()so child program arguments are never consumed by popl--netdev,--disable,--level) are handled viacount()/value(idx)loopspopl.hppwas vendored as a single header atpackage/include/popl.hpp— no new external dependency.Behavior
No functional changes intended. Verified:
ctest 14/14)--pidmode works-- prog ...mode worksThe CodeFactor issue is in popl.hpp itself (a vendored third-party header), not in any code introduced by this PR.