jarro2783 / cxxopts

Lightweight C++ command line option parser
MIT License
4.17k stars 582 forks source link

Add a no-argument constructor for ParseResult #146

Open Nikratio opened 5 years ago

Nikratio commented 5 years ago

I feel like I am missing something obvious, but how can I catch exceptions from parse() without having to put all my option handling code into the try..catch block?

C++ sadly isn't smart enough to make this work:

    cxxopts::Options opt_parser(argv[0]); 
    opt_parser.add_options() 
        // [...]
        ("help", "Print help");

    try {
        auto options = opt_parser.parse(argc, argv);
    } catch (cxxopts::option_not_exists_exception& exc) {
        std::cout << argv[0] << ": " << exc.message << std::endl;
        print_usage(argv[0]);
        exit(2);
    }
    // Can't access `options` here anymore, out of scope..

but it seems that the lack of a constructor makes the regular alternative fail as well:

    cxxopts::Options opt_parser(argv[0]); 
    opt_parser.add_options() 
        // [...]
        ("help", "Print help");

    cxxopts::ParseResult options;  /// FAILS!
    try {
        options = opt_parser.parse(argc, argv);
    } catch (cxxopts::option_not_exists_exception& exc) {
        std::cout << argv[0] << ": " << exc.what() << std::endl;
        print_usage(argv[0]);
        exit(2);
    }
    // Do stuff with `options`

What's the typical idiom to handle this?

I tried using a wrapper function:

static cxxopts::ParseResult parse_wrapper(
    cxxopts::Options parser, int argc, char** argv) {
    try {
        return parser.parse(argc, argv);
    } catch (cxxopts::option_not_exists_exception& exc) {
        std::cout << argv[0] << ": " << exc.what() << std::endl;
        print_usage(argv[0]);
        exit(2);
    }
}

but despite being rather guly, this also gives me a segfault when accessing the returned object - could there be some issue with the move constructor?

jarro2783 commented 5 years ago

Which version are you using? I recently fixed a segfault when the ParseResult goes out of scope of the Options object that created it.

What problem are you trying to solve that means that you can't wrap the whole thing in try, catch? You could handle everything as though there were no errors and wrap everything in exception handlers further up in the stack.

Nikratio commented 5 years ago

I'm using v2.1.1.

I'd rather not put everything into try..catch for stylistic reasons. None of the other operations could possibly raise this exception, so there is no point having them be in the try clause. Rather, this would introduce an extra indentation level and a needless distance (in code lines) between the code that may raise the exception and the code that handles it. Finally, some of that code also has its own exception handlers, and nesting try..catch is something that just feels wrong to me.

Handling the exception higher-up in the stack would work, but I am trying to write a function that encapsulates all the option handling (so having it raise the exception would defeat its purpose). Here is some more context:

static void parse_options(int argc, char **argv) {
    cxxopts::Options opt_parser(argv[0]);
    opt_parser.add_options()
        ("debug", "Enable debug messages from SteamFS")
        // [...]
        ("single", "Run single-threaded (for testing only)");

    try {
        auto options = opt_parser.parse(argc, argv);

        if (options.count("help")) {
            print_usage(argv[0]);
            // Strip everything before the option list from the default help string.
            auto help = opt_parser.help();
            std::cout << std::endl << "options:"
                      << help.substr(help.find("\n\n") + 1, string::npos);
            exit(0);

        } else if (argc != 3) {
            std::cout << argv[0] << ": invalid number of arguments\n";
            print_usage(argv[0]);
            exit(2);
        }

        // Open some files, validate some values, etc
    } catch (cxxopts::option_not_exists_exception& exc) {
        std::cout << argv[0] << ": " << exc.what() << std::endl;
        print_usage(argv[0]);
        exit(2);
    }
}

Moving the catch clause out of the function would be weird, because the exception handling code is so similar to the code handling --help and invalid arguments.

jarro2783 commented 5 years ago

What's wrong with handling the correct options after the comment at the bottom of the try?

That way you either return with everything handled correctly or exit with some error code.

Nikratio commented 5 years ago

What bothers me about putting the code into the bottom of the try is that:

jarro2783 commented 5 years ago

Thee problem with returning a ParseResult object out of the scope of Options is fixed in master. Would it work to return that result if all the checks passed, and exit otherwise? I think C++ understands that exit doesn't return, and you should be able to satisfy the type checker.

ParseResult parse_options(int argc, char** argv) {
  try {
    cxxopts::Options options;
    // set up options and parse here:

    auto result = options.parse(argc, argv);

    if (error) {
      exit(1);
    }
    return result;
  } catch (...) {
    exit(1);
  }
}
Nikratio commented 5 years ago

Thanks for your efforts! No, at least g++ 6 isn't smart enough for this.

However, is there a fundamental reason why the following could not be made to work?

    cxxopts::ParseResult options;  // Produce an empty object
    try {
        options = opt_parser.parse(argc, argv);  // copy/move the actual options
    } catch (cxxopts::option_not_exists_exception& exc) {
        std::cout << argv[0] << ": " << exc.what() << std::endl;
        print_usage(argv[0]);
        exit(2);
    }
    // Do stuff with `options`
jarro2783 commented 5 years ago

It could be made to work, but I just prefer not to have the possibility of uninitialised objects even existing. I suppose in this case an empty ParseResult just has no arguments stored in it, so in this case it would be reasonable to do that.

hhirsch commented 5 years ago

Will this feature be available anytime soon?

jarro2783 commented 5 years ago

I'll try to get this done in the next couple of weeks. It won't take long, I just have holidays and other things to do.

jarro2783 commented 5 years ago

I just had a look at the code and realised that some hack I did a while ago makes this kind of difficult. I would have to do a fairly big rewrite to get this to work.

I will definitely rewrite it at some point, because it should be done. But it will take a bit longer than I thought.

AntumDeluge commented 5 years ago

Glad this issue has been addressed, hope that it gets a fix soon. I am trying to do the same thing.

I am using version 2.2.0. Is there currently a way to initialize cxxopts::ParseResult without having to call cxxopts::Options.parse?

Here is my code:

int main(int argc, char** argv) {
    cxxopts::Options options(executable, "Convert binary files to C/C++ headers");
    options.add_options()
            ("h,help", "help")
            ("v,version", "version")
            ("s,chunksize", "Buffer chunk size", cxxopts::value<unsigned int>())
            ("stdvector", "vector");

    // FIXME: can't initialize empty cxxopts::ParseResult
    cxxopts::ParseResult args;
    try {
        args = options.parse(argc, argv);
    } catch (const cxxopts::OptionParseException& e) {
        exitWithError(e.what(), 1, true);
    }
...
...
}

This is the g++ compiler error output:

error: no matching function for call to 'cxxopts::ParseResult::ParseResult()'
   82 |  cxxopts::ParseResult args;
      |                       ^~~~
In file included from C:/Users/antum/Development/bin2header/source/src/bin2header.cpp:9:
C:/Users/antum/Development/bin2header/source/src/include/cxxopts.hpp:1497:1: note: candidate: 'cxxopts::ParseResult::ParseResult(std::shared_ptr<std::unordered_map<std::__cxx11::basic_string<char>, std::shared_ptr<cxxopts::OptionDetails> > >, std::vector<std::__cxx11::basic_string<char> >, bool, int&, char**&)'
 1497 | ParseResult::ParseResult
      | ^~~~~~~~~~~
C:/Users/antum/Development/bin2header/source/src/include/cxxopts.hpp:1497:1: note:   candidate expects 5 arguments, 0 provided
C:/Users/antum/Development/bin2header/source/src/include/cxxopts.hpp:1132:9: note: candidate: 'cxxopts::ParseResult::ParseResult(const cxxopts::ParseResult&)'
 1132 |   class ParseResult
      |         ^~~~~~~~~~~
C:/Users/antum/Development/bin2header/source/src/include/cxxopts.hpp:1132:9: note:   candidate expects 1 argument, 0 provided
C:/Users/antum/Development/bin2header/source/src/include/cxxopts.hpp:1132:9: note: candidate: 'cxxopts::ParseResult::ParseResult(cxxopts::ParseResult&&)'
C:/Users/antum/Development/bin2header/source/src/include/cxxopts.hpp:1132:9: note:   candidate expects 1 argument, 0 provided
C:/Users/antum/Development/bin2header/source/src/bin2header.cpp:84:34: error: use of deleted function 'cxxopts::ParseResult& cxxopts::ParseResult::operator=(cxxopts::ParseResult&&)'
   84 |   args = options.parse(argc, argv);
      |                                  ^
In file included from C:/Users/antum/Development/bin2header/source/src/bin2header.cpp:9:
C:/Users/antum/Development/bin2header/source/src/include/cxxopts.hpp:1132:9: note: 'cxxopts::ParseResult& cxxopts::ParseResult::operator=(cxxopts::ParseResult&&)' is implicitly deleted because the default definition would be ill-formed:
 1132 |   class ParseResult
      |         ^~~~~~~~~~~
C:/Users/antum/Development/bin2header/source/src/include/cxxopts.hpp:1132:9: error: no matching function for call to 'std::shared_ptr<std::unordered_map<std::__cxx11::basic_string<char>, std::shared_ptr<cxxopts::OptionDetails> > >::operator=(const std::shared_ptr<std::unordered_map<std::__cxx11::basic_string<char>, std::shared_ptr<cxxopts::OptionDetails> > >) const'
In file included from C:/Users/antum/Environment/MSYS2-x86_64/mingw64/include/c++/9.2.0/memory:81,
                 from C:/Users/antum/Development/bin2header/source/src/include/cxxopts.hpp:34,
                 from C:/Users/antum/Development/bin2header/source/src/bin2header.cpp:9:
C:/Users/antum/Environment/MSYS2-x86_64/mingw64/include/c++/9.2.0/bits/shared_ptr.h:309:19: note: candidate: 'std::shared_ptr<_Tp>& std::shared_ptr<_Tp>::operator=(const std::shared_ptr<_Tp>&) [with _Tp = std::unordered_map<std::__cxx11::basic_string<char>, std::shared_ptr<cxxopts::OptionDetails> >]' <near match>
  309 |       shared_ptr& operator=(const shared_ptr&) noexcept = default;
      |                   ^~~~~~~~
C:/Users/antum/Environment/MSYS2-x86_64/mingw64/include/c++/9.2.0/bits/shared_ptr.h:309:19: note:   passing 'const std::shared_ptr<std::unordered_map<std::__cxx11::basic_string<char>, std::shared_ptr<cxxopts::OptionDetails> > >*' as 'this' argument discards qualifiers
C:/Users/antum/Environment/MSYS2-x86_64/mingw64/include/c++/9.2.0/bits/shared_ptr.h:313:2: note: candidate: 'std::shared_ptr<_Tp>::_Assignable<const std::shared_ptr<_Yp>&> std::shared_ptr<_Tp>::operator=(const std::shared_ptr<_Yp>&) [with _Yp = std::unordered_map<std::__cxx11::basic_string<char>, std::shared_ptr<cxxopts::OptionDetails> >; _Tp = std::unordered_map<std::__cxx11::basic_string<char>, std::shared_ptr<cxxopts::OptionDetails> >; std::shared_ptr<_Tp>::_Assignable<const std::shared_ptr<_Yp>&> = std::shared_ptr<std::unordered_map<std::__cxx11::basic_string<char>, std::shared_ptr<cxxopts::OptionDetails> > >&]' <near match>
  313 |  operator=(const shared_ptr<_Yp>& __r) noexcept
      |  ^~~~~~~~
C:/Users/antum/Environment/MSYS2-x86_64/mingw64/include/c++/9.2.0/bits/shared_ptr.h:313:2: note:   passing 'const std::shared_ptr<std::unordered_map<std::__cxx11::basic_string<char>, std::shared_ptr<cxxopts::OptionDetails> > >*' as 'this' argument discards qualifiers
C:/Users/antum/Environment/MSYS2-x86_64/mingw64/include/c++/9.2.0/bits/shared_ptr.h:324:2: note: candidate: 'template<class _Yp> std::shared_ptr<_Tp>::_Assignable<std::auto_ptr<_Up> > std::shared_ptr<_Tp>::operator=(std::auto_ptr<_Up>&&) [with _Yp = _Yp; _Tp = std::unordered_map<std::__cxx11::basic_string<char>, std::shared_ptr<cxxopts::OptionDetails> >]'
  324 |  operator=(auto_ptr<_Yp>&& __r)
      |  ^~~~~~~~
C:/Users/antum/Environment/MSYS2-x86_64/mingw64/include/c++/9.2.0/bits/shared_ptr.h:324:2: note:   template argument deduction/substitution failed:
In file included from C:/Users/antum/Development/bin2header/source/src/bin2header.cpp:9:
C:/Users/antum/Development/bin2header/source/src/include/cxxopts.hpp:1132:9: note:   types 'std::auto_ptr<_Up>' and 'const std::shared_ptr<std::unordered_map<std::__cxx11::basic_string<char>, std::shared_ptr<cxxopts::OptionDetails> > >' have incompatible cv-qualifiers
 1132 |   class ParseResult
      |         ^~~~~~~~~~~
In file included from C:/Users/antum/Environment/MSYS2-x86_64/mingw64/include/c++/9.2.0/memory:81,
                 from C:/Users/antum/Development/bin2header/source/src/include/cxxopts.hpp:34,
                 from C:/Users/antum/Development/bin2header/source/src/bin2header.cpp:9:
C:/Users/antum/Environment/MSYS2-x86_64/mingw64/include/c++/9.2.0/bits/shared_ptr.h:333:7: note: candidate: 'std::shared_ptr<_Tp>& std::shared_ptr<_Tp>::operator=(std::shared_ptr<_Tp>&&) [with _Tp = std::unordered_map<std::__cxx11::basic_string<char>, std::shared_ptr<cxxopts::OptionDetails> >]' <near match>
  333 |       operator=(shared_ptr&& __r) noexcept
      |       ^~~~~~~~
C:/Users/antum/Environment/MSYS2-x86_64/mingw64/include/c++/9.2.0/bits/shared_ptr.h:333:7: note:   conversion of argument 1 would be ill-formed:
In file included from C:/Users/antum/Development/bin2header/source/src/bin2header.cpp:9:
C:/Users/antum/Development/bin2header/source/src/include/cxxopts.hpp:1132:9: error: binding reference of type 'std::shared_ptr<std::unordered_map<std::__cxx11::basic_string<char>, std::shared_ptr<cxxopts::OptionDetails> > >&&' to 'const std::shared_ptr<std::unordered_map<std::__cxx11::basic_string<char>, std::shared_ptr<cxxopts::OptionDetails> > >' discards qualifiers
 1132 |   class ParseResult
      |         ^~~~~~~~~~~
In file included from C:/Users/antum/Environment/MSYS2-x86_64/mingw64/include/c++/9.2.0/memory:81,
                 from C:/Users/antum/Development/bin2header/source/src/include/cxxopts.hpp:34,
                 from C:/Users/antum/Development/bin2header/source/src/bin2header.cpp:9:
C:/Users/antum/Environment/MSYS2-x86_64/mingw64/include/c++/9.2.0/bits/shared_ptr.h:341:2: note: candidate: 'template<class _Yp> std::shared_ptr<_Tp>::_Assignable<std::shared_ptr<_Yp> > std::shared_ptr<_Tp>::operator=(std::shared_ptr<_Yp>&&) [with _Yp = _Yp; _Tp = std::unordered_map<std::__cxx11::basic_string<char>, std::shared_ptr<cxxopts::OptionDetails> >]'
  341 |  operator=(shared_ptr<_Yp>&& __r) noexcept
      |  ^~~~~~~~
C:/Users/antum/Environment/MSYS2-x86_64/mingw64/include/c++/9.2.0/bits/shared_ptr.h:341:2: note:   template argument deduction/substitution failed:
In file included from C:/Users/antum/Development/bin2header/source/src/bin2header.cpp:9:
C:/Users/antum/Development/bin2header/source/src/include/cxxopts.hpp:1132:9: note:   types 'std::shared_ptr<_Tp>' and 'const std::shared_ptr<std::unordered_map<std::__cxx11::basic_string<char>, std::shared_ptr<cxxopts::OptionDetails> > >' have incompatible cv-qualifiers
 1132 |   class ParseResult
      |         ^~~~~~~~~~~
In file included from C:/Users/antum/Environment/MSYS2-x86_64/mingw64/include/c++/9.2.0/memory:81,
                 from C:/Users/antum/Development/bin2header/source/src/include/cxxopts.hpp:34,
                 from C:/Users/antum/Development/bin2header/source/src/bin2header.cpp:9:
C:/Users/antum/Environment/MSYS2-x86_64/mingw64/include/c++/9.2.0/bits/shared_ptr.h:349:2: note: candidate: 'template<class _Yp, class _Del> std::shared_ptr<_Tp>::_Assignable<std::unique_ptr<_Up, _Ep> > std::shared_ptr<_Tp>::operator=(std::unique_ptr<_Up, _Ep>&&) [with _Yp = _Yp; _Del = _Del; _Tp = std::unordered_map<std::__cxx11::basic_string<char>, std::shared_ptr<cxxopts::OptionDetails> >]'
  349 |  operator=(unique_ptr<_Yp, _Del>&& __r)
      |  ^~~~~~~~
C:/Users/antum/Environment/MSYS2-x86_64/mingw64/include/c++/9.2.0/bits/shared_ptr.h:349:2: note:   template argument deduction/substitution failed:
In file included from C:/Users/antum/Development/bin2header/source/src/bin2header.cpp:9:
C:/Users/antum/Development/bin2header/source/src/include/cxxopts.hpp:1132:9: note:   types 'std::unique_ptr<_Tp, _Dp>' and 'const std::shared_ptr<std::unordered_map<std::__cxx11::basic_string<char>, std::shared_ptr<cxxopts::OptionDetails> > >' have incompatible cv-qualifiers
 1132 |   class ParseResult
      |         ^~~~~~~~~~~

Edit: Came up with a little workaround:

DELETED (see end of post for fixed version)

Only issue with this is a little warning the compiler spits out because I can't return an instance of cxxopts::ParseResult outside the try statement at the end of parseArgs function. But it works.

C:/Users/antum/Development/bin2header/source/src/bin2header.cpp: In function 'cxxopts::ParseResult parseArgs(cxxopts::Options, int, char**)':
C:/Users/antum/Development/bin2header/source/src/bin2header.cpp:70:48: warning: control reaches end of non-void function [-Wreturn-type]
   70 |  } catch (const cxxopts::OptionParseException& e) {
      |

Note: Appears to be the same, or similar, to the wrapper function that @Nikratio came up with. Not sure why it didn't work for him. Perhaps an older version?

Edit: Oh, @jarro2783, I forgot to thank you for the great software 😉. This has made command line parsing bearable. I was surprised at how quickly I was able to get it to work. So from a person with poor coding skills, thank you very much.

Edit: Oooops, I need to pass argc & argv by reference to the function:


/** Parses command line arguments & returns cxxopts::ParseResult. */
cxxopts::ParseResult parseArgs(cxxopts::Options opts, int* argc, char*** argv) {
    try {
        cxxopts::ParseResult res = opts.parse(*argc, *argv);
        return res;
    } catch (const cxxopts::OptionParseException& e) {
        exitWithError(e.what(), 1, true);
    }
}

int main(int argc, char** argv) {
    cxxopts::Options options(executable, "Convert binary files to C/C++ headers");
    options.add_options()
            ("h,help", "help")
            ("v,version", "version")
            ("s,chunksize", "Buffer chunk size", cxxopts::value<unsigned int>())
            ("stdvector", "vector");

    cxxopts::ParseResult args = parseArgs(options, &argc, &argv);
...
...
}```