HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Command line arguments adhering Posix and GNU standards

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
argumentslineposixadheringstandardsandcommandgnu

Problem

The following code extracts command line parameters and modifies 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 argc and argv?



  • How to make clear to the caller they change?



  • Are the bool and std::string specialisations 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 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 baz


The 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] = baz


The 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 baz
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] = baz

Context

StackExchange Code Review Q#160624, answer score: 2

Revisions (0)

No revisions yet.