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.
No functional changes were made.
|
@graeme-a-stewart Thanks for the Review
No functional changes were made. |
graeme-a-stewart
left a comment
graeme-a-stewart
left a comment
There was a problem hiding this comment.
Thanks for all the changes @iareARiES. Can you also modify the invocation of a child process and its arguments to use pool's non_option_args().
| std::cerr << "Use '--help' for usage " << std::endl; | ||
| return 1; | ||
| // Scan argv for "--" separator before popl parsing | ||
| // so that child program arguments are hidden from the option parser |
There was a problem hiding this comment.
I realised that we don't need to do this "by hand". I believe that popl supports the "--" separator and makes these arguments available through non_option_args(). See:
There was a problem hiding this comment.
BTW, this was not very evident from any documentation that I saw, but looking at the code I am pretty sure it is there and should work
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.