Proposed update to remove RAII problem

[trick2011]

I propose the removal of the use of std::exit. It's usage breaks RAII which is not mentioned in the documentation.

As for the addition of the exception, during parsing we already require that the user catches several exceptions thus the whole try/catch harness is already there for a proper program. Adding a catch clause for the new NormalProgramTermination is a bit bulky but doesn't break RAII and is explicit on that the program should terminate in normal usage.

[skrobinson]

I disagree with this change. This requires every program without overridden "--help" and "--version" to explicitly handle an exception for non-exceptional usage. The current behavior is to let basic users focus on their arguments and allow advanced users to override the default handlers for advanced needs.

Yes, std::exit does not call dtors, etc. But, I would argue that if someone has special clean-up procedures they should be overriding the --help and --version defaults.

[trick2011]

RAII is not a not special clean-up procedure and not executing dtors is a critical breakage with core functionality of C++. It at the very least should be explicitly clear that dtors might not be called in some execution paths of the library.

Maybe the snippet below would be possible as a usage pattern. This allows termination without exceptions but doesn't do it by calling std::exit.

try{
    if(program.parse_args(argc, argv)){
        // program flow
    }
}
catch(const std::invalid_argument &err){

}

Sure it requires setup by the user, that is however the tools that the languages gives us to handle these scenario's and to reiterate, std::exit breaks a lot of stuff.

[marzer]

FWIW I agree with this change; I ran into this problem in my own program and thought it was a very goofy part of the design - not honouring idiomatic RAII by default is just confusing C++.

[p-ranav]

Hi. Thanks for the PR and the comments. Your points regarding RAII are valid.

One of the reasons why this library chooses to exit after printing help is because Python's argparse exits after printing help. As you can imagine, much of the API is designed based on Python's argparse.

We call exit; we don't call abort. exit is considered a "normal" end of the program, although this may still indicate a failure (but not a bug). abort is considered an "abnormal" end to the program and raises SIGABRT.

Yes, it is bad that destructors are not called when this happens, I don't deny this.

We already have a constructor argument to include or exclude default arguments (help, version). How about adding a second argument that configures whether or not the library will exit the program? This can be documented up front. Python's argparse has a similar exit_on_error constructor argument (default: true).

explicit ArgumentParser(std::string program_name = {},
                        std::string version = "1.0",
                        default_arguments add_args = default_arguments::all,
                        exit_on_default_arguments = true)

If we allow the user to configure whether or not exit will happen, the user can then handle this issue however they see fit. As @skrobinson mentioned, the user can already override the behavior today by providing their own version of these default arguments which does not exit the program but instead throws a custom exception. There doesn't seem to be a clear reason to add this custom exception type to the library.

I'd be happy to document that the default behavior of the program is to exit and that there are ways to prevent that from happening.

[marzer]

There doesn't seem to be a clear reason to add this custom exception type to the library.

I'd argue that "do the correct C++ thing by default" is reason enough, personally. Copying python's argparse breaks down once you run afoul of the fact that C++ is not, in fact, Python, and thus has different semantics. If the current behaviour is to be preserved, there should at least be a warning in big red letters in the README and elsewhere.

[trick2011]

I agree with @marzer, we are coming up against a difference between what C++ and Python can do. Proper handling in C++ means doing it in a different way than Python. It is of-course always preferred if a codepath can be handled internally but C++ doesn't allow for that in this situation.

Users of C++ are accustomed to having to handle details sometimes and providing a easy, clear, concise and proper way of doing it should be the priority. Doing it wrong by default isn't right. It also teaches others to disregard the core components of C++ for a reduction in outer handling code.

Adding a library internal exitRequested flag which is to be set in the -h and -v functions would allow for communicating this in the argparse return value.

[trick2011]

An argument in favor of using the error handling scheme of exceptions is that they are made to keep our programs in a determinant state by allowing for cleanup when program termination is needed. Normally we need proper program termination because of an egregious program error internally. In this situation it is 'normal' behaviour we want and everything went correctly, however we still require by default that a program terminates when called with the -h and -v flags. It is therefore an 'error' in the sense that the program shouldn't go on further than this point and should terminate correctly. The user needs to provide the proper inputs for the program to progress deeper into its flow.

[skrobinson]

RAII is not a not special clean-up procedure

What I meant is that for many cases, exiting the process and returning the program's resources to the host environment is sufficient.

Funnily enough, I don't disagree with most of your argument. But, I see our current behavior as a way to make simple cases simple in a way that many potential users will not find surprising. Using the name argparse signals to potential users what behavior to generally expect.

Since PR #142, we have allowed the default arguments to be disabled by those users that have different needs or preferences.

[marzer]

If you're going to stick with this as default, the RAII implications need to be documented accordingly. Simply making the word exit bold is insufficient. An entire separate paragraph in bold, beginning with

⚠️️WARNING:

would be the minimum expectation for something so non-idiomatic, imo.

[marzer]

What I meant is that for many cases, exiting the process and returning the program's resources to the host environment is sufficient.

Also, as software becomes increasingly cross-platform, this assertion is misguided, and I submit that it's actually the majority that will need proper handling, even without realising. Many applications need to initialise the terminal (for coloured output, virtual terminal on windows, Unicode handling modes, etc), and generally that needs to happen before argparse is even instantiated. A good program will then reverse these changes on exit so that the user's terminal is left in the same state it was at the beginning. C++ RAII is a perfect way to handle this robustly.

[trick2011]

Using the name argparse signals to potential users what behavior to generally expect.

I understand this drive and I fully agree with it. Having used both, I can see how well it was achieved. We however need to recon with the fact that python and C++ are fundamentally different in some aspects.

Another possibility is to introduce non portable code, e.g. calling kill on it's own process when using linux.

Simply making the word exit bold is insufficient.

Not to mention that this is half way down the instructions buried under an obscure header of "default arguments" and doesn't mention anything about the implications for RAII.

[skrobinson]

You've both argued so strongly for this that I re-examined my view on the general matter of std::exit. But, I have not changed my opinion.

std::exit is normal termination. But, std::exit does not work for every program's needs. std::exit from --help is only a default and argparse provides for disabling/replacing default arguments.

Trying to improve RAII support by throwing on --help means that every program will need to handle exceptions and a large number will need to explicitly exit after --help. This is a complication that is not needed by many programs.

To be explicitly clear, I don't believe you're wrong. Instead, this is a design decision that attempts to start simple while allowing more rigorous use where needed.

[marzer]

std::exit is normal termination.

For some definition of normal. It skips a normal part of C++, so I'd say very definitively no it is not. It's normal from the OS's perspective, and that's all. Meaningless from the perspective of the points being raised here.

But, I have not changed my opinion.

That is unfortunate, because this opinion will open this library up to being the source of one or more CVEs. It probably already has, indirectly.

If you/the library owner insist on maintaining this stance, then I'd like to reiterate that the existing documentation is woefully insufficient on this point. It should be very clearly demarcated with separate warning paragraph in the README, e.g.:

---

⚠️ RAII WARNING: Exiting from the default help dialog is performed with std::exit() for convenience. Exiting a program in this fashion does not honor RAII (i.e. destructors are not executed), so if you do any non-trivial initialization before invoking argparse (e.g. creating a lock file), you must ensure you are able to manually perform cleanup operations where necessary using std::atexit().

---

I'd also recommend you replace all appearances of std::exit(1) that appear in main() throughout the README with simply return 1;, because the former is overkill and would not likely survive a code review. Use of std::exit() should not be encouraged where there is a safer alternative.

(I am also happy to make these documentation changes myself via a PR, of course.)

[skrobinson]

Why do you recommend using std::atexit() vs customizing the --help argument?

To repeat: std::exit is the default, not a requirement. Devs can replace the default --help during argparse construction to have any behavior a project wants, including throwing.

I'd also recommend you replace all appearances of std::exit(1) that appear in main() throughout the README with simply return 1;

You make a good point here. I'd support a PR with that change. The samples directory should also be updated.

[marzer]

Why do you recommend using std::atexit() vs customizing the --help argument?

I'm not necessarily recommending one over the other, I'm saying the README should appropriately cover both scenarios, including caveats. Currently it does not do so sufficiently.

Customizing --help requires you to add a new argument and configure it appropriately, which is an inconvenience. I know that argparse provides operator << to assist with this, but it is an inconvenience all the same. We programmers are a lazy bunch; undoubtedly most will choose to use the default --help over having to actually re-implement it. They should be appropriately informed of the RAII implications of that choice. That's what I'm suggesting, in lieu of changing the implementation.

To repeat: std::exit is the default [...]

I understand that. Comprehension is not the issue. The problem is that it's a bad default. The default behaviour should be the safe behaviour. A some-time-in-the-future major version bump of the library should change this, IMO. Currently it's trying just a bit too hard to copy Python.

There's a backwards-compatible middle-ground where you get the best of both worlds that can be implemented without using an exception (as that seems to be a sticking point here): The parser object could store some state allowing programmer to query if the user invoked one of the defaults, combined with a flag that tells the parser not to exit automatically (@p-ranav's exit_on_default_arguments suggestion above, or a new default_arguments flag), e.g.:

argparse::ArgumentParser program(
	"compiler",
	"1.0",
	default_arguments::all | default_arguments::do_not_exit
);

program.parse_args(argc, argv);

if (program.invoked_default_argument())
	return 0;

This is vastly superior IMO, because you still get all the convenience of the default --help without having to re-implement it, but also:

• RAII still works
• Choose your own exit code
• Can do additional stuff after handling the default arguments without requring nonsense like std::atexit
• Could even choose to simply not exit at all if that made sense for the application

You make a good point here. I'd support a PR with that change.

FYI if I make a PR with that change, my PR would also include the RAII warning. I don't think it would be all that useful without it.

[skrobinson]

I've made a first-draft implementation of @p-ranav's exit_on_default_arguments idea at skrobinson/argparse@59286f01. Thoughts?

[marzer]

Looks good so far, but you need to also pair it with some way of querying whether a built-in default argument was invoked (e.g. my invoked_default_argument() example above), otherwise it's not really very useful on it's own.

EDIT: actually, does is_used() work for the built-in default arguments? e.g. parser.is_used("--version")? if so then it's probably fine as-is

[skrobinson]

Yes, is_used will work. Your example could be writen as...

program.parse_args(argc, argv);

if (program.is_used("--help") or program.is_used("--version"))
    return 0;

[marzer]

Well then, consider me on board, heh. That's a good alternative for people not wanting a forced std::exit().

[skrobinson]

@trick2011 Does PR #264 meet your needs?

[trick2011]

@skrobinson #264 is a fine step forwards to a better version and I definitely approve of its inclusion in the main codebase.

I still disagree with the default behaviour being a call to exit but sometimes you win some and you lose some.