patterncppMinor
Command line arguments adhering Posix and GNU standards
Viewed 0 times
argumentslineposixadheringstandardsandcommandgnu
Problem
The following code extracts command line parameters and modifies
Please keep following questions in mind while reviewing:
Condensed:
```
/**
* Basic functions shared by general template and specialisations
*/
template
class CliArgMixin
{
protected:
/**
* Helper function to delete arguments.
*/
static void deleteArg(int& argc, char* argv[], int pos)
{
assert(argv);
assert(argc > 0);
if(pos >= argc) return;
for(int i=pos; i 0);
if(name.empty()) return false;
std::string entry;
for(int i=0; i 0);
assert(argv);
if(name.empty()) return;
std::string entry;
for(int i=0; i
class CliArg : public CliArgMixin
{
typedef CliArgMixin Mixin;
public:
CliArg(int& argc, char* argv[], const std::string& opt, const std::string& longOpt, const T& d = T())
: Mixin(d)
{
if(opt.length() > 1)
{
throw std::invalid_argument("Short option must be one character or empty");
}
if(longOpt.length() == 1)
{
throw std::invalid_argument("Long option must not be one character");
}
if( Mixin::search(argc, argv, longOpt) == false )
{
if( Mixin::search(argc, argv, opt) == false )
{
Mixin::searchCondensed(argc, argv, opt);
}
}
}
~CliArg() {}
const T& value() const { return CliArgMixin::m_value; }
operator const T&() const { return CliArgMixin::m_value; }
private:
CliArg() = delete;
virtual void extract(int& argc, char* argv[], int index)
{
argc and argv parameters of the. After all parameters are read remaining values (especially after --) are remaining.Please keep following questions in mind while reviewing:
- Is it good practice to modify
argcandargv?
- How to make clear to the caller they change?
- Are the
boolandstd::stringspecialisations obvious?
- Is there a shorter/compacter way than using the currently used mixin?
Condensed:
```
/**
* Basic functions shared by general template and specialisations
*/
template
class CliArgMixin
{
protected:
/**
* Helper function to delete arguments.
*/
static void deleteArg(int& argc, char* argv[], int pos)
{
assert(argv);
assert(argc > 0);
if(pos >= argc) return;
for(int i=pos; i 0);
if(name.empty()) return false;
std::string entry;
for(int i=0; i 0);
assert(argv);
if(name.empty()) return;
std::string entry;
for(int i=0; i
class CliArg : public CliArgMixin
{
typedef CliArgMixin Mixin;
public:
CliArg(int& argc, char* argv[], const std::string& opt, const std::string& longOpt, const T& d = T())
: Mixin(d)
{
if(opt.length() > 1)
{
throw std::invalid_argument("Short option must be one character or empty");
}
if(longOpt.length() == 1)
{
throw std::invalid_argument("Long option must not be one character");
}
if( Mixin::search(argc, argv, longOpt) == false )
{
if( Mixin::search(argc, argv, opt) == false )
{
Mixin::searchCondensed(argc, argv, opt);
}
}
}
~CliArg() {}
const T& value() const { return CliArgMixin::m_value; }
operator const T&() const { return CliArgMixin::m_value; }
private:
CliArg() = delete;
virtual void extract(int& argc, char* argv[], int index)
{
Solution
This is nice, neat code in my view, and there is absolutely nothing wrong with modifying
Provide complete code to reviewers
The code as presented is not quite complete. It is missing these necessary includes and does not have a
I moved everything to a file
With that added, it was easier to review the code.
Provide an operator for legacy C functions
The
Consider repeated arguments
It's common for repeated arguments to have a defined behavior, such as "only the last one has effect" or "only the first one has effect." This code uses, in effect, the latter policy but then does not ignore or process any subsequent repeats of that option, leaving the user of this code to clean that up. It would be nice if the code handled that.
Fix the bug
The handling of combined flags, such as
The rather peculiar effect is this:
The second
Think of the user
With a long list of arguments, it would be rather tedious to use this syntax repeatedly:
It might be nice to allow for a less verbose syntax or one that would support initialization of multiple variables, possibly with default values, via a
Check your spelling
In one of the comments, there's a typo: "condesed". Since this is generally nice code, it would be worth the extra effort to make sure the comments don't have spelling errors. That's the only spelling error I noticed.
argc and argv. Indeed, it's an entirely rational and natural way to do things for just this kind of purpose. Here are some things that may help you further improve your code:Provide complete code to reviewers
The code as presented is not quite complete. It is missing these necessary includes and does not have a
main to exercise the functions.#include
#include
#include
#include
#include
#include I moved everything to a file
args.h and wrote this to test:#include "args.h"
#include
#include
#define SHOW(X) std::cout isVerbose(argc, argv, "v", "verbose");
CliArg inFileName(argc, argv, "f", "filename");
CliArg count(argc, argv, "n", "number");
SHOW(isVerbose);
SHOW(inFileName);
SHOW(count);
std::cout << "Remaining args:\n";
for (int i{0}; i < argc; ++i) {
std::cout << "argv[" << i << "] = " << argv[i] << '\n';
}
}With that added, it was easier to review the code.
Provide an operator for legacy C functions
The
std::string overload is very likely to be used for things such as file names that might be passed to legacy C functions, so to make that more convenient, I'd suggest adding one more function:operator const char *() const { return Mixin::m_value.c_str(); }Consider repeated arguments
It's common for repeated arguments to have a defined behavior, such as "only the last one has effect" or "only the first one has effect." This code uses, in effect, the latter policy but then does not ignore or process any subsequent repeats of that option, leaving the user of this code to clean that up. It would be nice if the code handled that.
Fix the bug
The handling of combined flags, such as
'-vf' in the sample code above seems to work, but has a flaw having to do with unknown or repeated arguments. If we run the code as:src/args -n 42 -vvf config.log foo bar biz bazThe rather peculiar effect is this:
isVerbose = true
inFileName = v
count = 42
Remaining args:
argv[0] = src/args
argv[1] = config.log
argv[2] = foo
argv[3] = bar
argv[4] = biz
argv[5] = bazThe second
v is misinterpreted as the inFileName while the actual file name is unprocessed.Think of the user
With a long list of arguments, it would be rather tedious to use this syntax repeatedly:
CliArg isVerbose(argc, argv, "v", "verbose");
CliArg isCompressed(argc, argv, "c", "compressed");
CliArg filterName(argc, argv, "f", "filename");
CliArg waveletType(argc, argv, "w", "wavelet");
CliArg count(argc, argv, "n", "number");It might be nice to allow for a less verbose syntax or one that would support initialization of multiple variables, possibly with default values, via a
constexpr data structure.Check your spelling
In one of the comments, there's a typo: "condesed". Since this is generally nice code, it would be worth the extra effort to make sure the comments don't have spelling errors. That's the only spelling error I noticed.
Code Snippets
#include <iostream>
#include <cassert>
#include <cstring>
#include <stdexcept>
#include <string>
#include <sstream>#include "args.h"
#include <iostream>
#include <iomanip>
#define SHOW(X) std::cout << std::boolalpha << # X " = " << (X) << '\n'
int main(int argc, char *argv[]) {
CliArg<bool> isVerbose(argc, argv, "v", "verbose");
CliArg<std::string> inFileName(argc, argv, "f", "filename");
CliArg<int> count(argc, argv, "n", "number");
SHOW(isVerbose);
SHOW(inFileName);
SHOW(count);
std::cout << "Remaining args:\n";
for (int i{0}; i < argc; ++i) {
std::cout << "argv[" << i << "] = " << argv[i] << '\n';
}
}operator const char *() const { return Mixin::m_value.c_str(); }src/args -n 42 -vvf config.log foo bar biz bazisVerbose = true
inFileName = v
count = 42
Remaining args:
argv[0] = src/args
argv[1] = config.log
argv[2] = foo
argv[3] = bar
argv[4] = biz
argv[5] = bazContext
StackExchange Code Review Q#160624, answer score: 2
Revisions (0)
No revisions yet.