Raii

[ferkulat]

With this you don't have to manage heap memory manually anymore and its exception save.

Could you please have a look at 33fffb6

[FlorianRappl]

In general looks good to me.

Regarding the question: The problem here is / was to stop the program execution if the "help option" was detected. Now this changed to be implemented via a custom exception. No matter if we return true or false, the execution will continue. We could also exit here, but potentially this is also not an ideal solution (was always a kind of workaround; obviously it is not good / easy to test, and not as flexible as potentially desired). If the run_and_exit_if_error method is used, we should return false. However, the help is a legit option and does is no real error (so true is the real result). Nevertheless, returning true will obviously treat everything is "alright", and thus prevent the program from exiting.

Long story short: I think neither true nor false are suitable here. false is a lie, true will introduce a bug / unwanted behavior. I would leave this untouched for now and tackle it sometime later (I think we would need a better concept here).

[ferkulat]

how about returning an enum class like

enum class {FAIL, EXIT_WITH_SUCCESS, SUCCESS}

The names are a quick shot, but you get the idea.

[FlorianRappl]

Yes, I was thinking the same, but the question is - how does it play with a method like run_and_exit_if_error . It would still continue. How would the user know if he has to stop program execution and why? Maybe you have a good idea for an API that is sound and easy.

[ferkulat]

In my opinion, it is not the responsibility of the parser to exit the program.
Just parse and return success/failure.
And the user of this parser makes a decision depending on the parser result.
In example throwing an exception or exit his program with exit(int).

My suggestion would be, to remove run_and_exit_if_error from the parser.

[FlorianRappl]

Of course, you are right that this is not the main responsibility. However, for convenience this method was introduced and is used quite often as far as I can see.

Regarding the enum - on second thought we have actually flags. We have

• Success / Failure (in parsing)
• Continue / Exit (overall result)

The happy path is either success with continue or failure with exit, however, we can also have success with exit or failure with continue (the latter could be that some parsing condition fails, but since the argument is optional this could be ignored; currently not implemented but it is a possibility).

[ferkulat]

Today I read about signal handling.
It seems it is not that portable, as it could.
A signal (like from exit(int)) gets emitted no destructors will get called.
Maybe this does not hurt for heap memory for small programs.
I don't know if the not freed memory is lost until next reboot.

What if the system is supposed to not reboot for months/years?
What is, if the user of the parser manages other resources ( i.e. files, network)?
With the parser calling exit() the user is forced to do proper signal handling to free his resources.
Does she/he know?

I really have mixed feelings about this exit() stuff.
If you want the parser influence the control flow, I would suggest exceptions.
These are portable and destructors will get called properly.

Initially I wanted to use your parser for my csv2xls.
I enjoyed working on the parser and I learned new things.
Thank you for that.
But as long as it emits signals, I don't want to use it.

With kind regards

[FlorianRappl]

All OSs free the resources allocated from an application when it exits. Otherwise, we would be doomed. exit is a sane and natural call.

And I think you misunderstood - its an opt-in and the parser does not exit for you if you do not wish to do that. It's a convenience method that can be used. But feel free to use something more appropriate for your workflow! Thanks for contributing 👍 !

[ferkulat]

Feel free to merge the PR anyway or at least cherrypick 42320bf please.

[Lecrapouille]

Hi @ferkulat and @FlorianRappl
I did not see this PR before opening issue #15 which is double post. Firstly, sorry for my bad english. My opinions on:

• Concerning ferkulat's idea to use exception for avoiding the exit:

Instead of exception, why not simply add a destroy() private method called by 1/ the class destructor and 2/ explicitly in before the exit(). As second alternative (but I do not like) you can use a "Proxy" class referring to class to be destroyed and before exit() you call delete on the Proxy. To heavy here !

• Concerning the fact to use unique_ptr instead of raw pointers: I think it's a bad idea because of using the get() method for returning the raw pointer. Bad practice to give an unprotected pointer to an unknown class which can may delete it. So false security.

The idea would be not storing pointers in vector but directly the class, implement the movable constructor, and let the explicit move() operator. After all your project in >= C++11

• Deleting resource before vector erase

Seems ok to me but with adding () to delte ?

[ferkulat]

Hello @Lecrapouille ,

unknown class which can may delete it. So false security.

Raw pointers are not bad by default.
Transferring owner ship should not be done with them.
If you get a raw pointer, you do not own it and should not delete it.
Please take a look at http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Ri-raw

[ferkulat]

Hello @Lecrapouille,

The idea would be not storing pointers in vector but directly the class

Since runtime polymorphism is used in this project, you will have to stick with pointers.