Diagnostic of errors.

[Alex Bukreev <bucreev@…>]

1. Some classes of errors (for example "unknown_option") are not remembered parameters transferred in the constructor. It does not allow to generate non standard diagnostics (for example not in English).
2. In a class "multiple_occurrences" there is no diagnostics for what parameter the error is found out.

[samjmill@…]

added myself

[Sascha Ochsenknecht <s.ochsenknecht@…>]

patch

[Sascha Ochsenknecht <s.ochsenknecht@…>]

I just attached a patch.

It adds the member function get_option_name() to the exceptions: unknown_option, multiple_values, multiple_occurrences and validation_error

The option name is set when exception is thrown, this gives the possibility for an appropriate error reporting when catching these exceptions.

I also added a new test case program_options/test/exception_test.cpp which checks some exceptions. This test is not fully complete but a good place to add checks related to exceptions.

Please comment.

[Vladimir Prus]

Sascha,

thanks for the patch. It's very nice, and I only found minor formatting things:

1. You seem to add an empty line to the 'error' class definition. I've removed it.
2. When you add set_option_name in the cpp file, you leave two empty lines between functions, while the rest of the code uses 1.

[Vladimir Prus]

(In [57746]) Add option name to a few exception classes.

Fixes #3423. Patch from Sascha Ochsenknecht.

[Alex Bukreev <bucreev@…>]

I’ve looked through your changes. There are 12 classes declared in file “errors.hpp”.

1. class “error” – base class, exception in this class are not generated (i suggest to declare its constructor as ‘protected’).

2. class “invalid_syntax” – there is a “msg” parameter of type ‘string’ which is specified in constructor, it is necessary to swap its parameter type to ‘enum’ with storing of its value (like it’s done in class “invalid_command_line_syntax”).

3. class “unknown_option” – ok.

4. class “ambiguous_option” - – there is a “name” parameter specified in constructor. Parameter value must be stored.

5. class "multiple_values" - there is a “what” parameter specified in constructor which is always equal to "multiple_values" parameter.

• remove “what” parameter from constructor.

6. class "multiple_occurrences" - there is a “what” parameter specified in constructor which is always equal to "multiple_values" parameter.

• remove “what” parameter from constructor.

7. class "validation_error - there is a “what” parameter of type ‘string’ specified in constructor. It is necessary to swap its parameter type to ‘enum’ with storing of its value.

8. class "invalid_option_value" - value of parameter "bad_value" is not stored.

9. class "too_many_positional_options_error" – if this exception may occur only because of programmer(not user) mistake, then it’s ok.

10. class "too_few_positional_options_error" - if this exception may occur only because of programmer(not user) mistake, then it’s ok.

11. class "invalid_command_line_syntax" – ok

12. class "invalid_command_line_style" - if this exception may occur only because of programmer(not user) mistake, then it’s ok.

While testing of “error” classes were detected the following: 1.Source one:

options_description desc; desc.add_options()

("cfgfile,c", value<string>()->multitoken(), "the config file") ("output,c", value<string>(), "the output file") ("output,o", value<string>(), "the output file")

;

const char* cmdline[] = {"program", "-c", "file", "-o", "anotherfile"};

No mistake detected!!!

2.Source two:

desc.add_options()

("cfgfile,c", value<string>()->multitoken(), "the config file") ("output,o", value<string>(), "the output file")

;

const char* cmdline[] = {"program", "-c", "-o", "anotherfile"};

Wrong diagnostic: "in option 'cfgfile': multiple values not allowed"

[Sascha Ochsenknecht]

I hope I cleaned it up a bit. I divided this "workpackage" into two parts to avoid big changes in one commit. So, first part is cleaning up of the exception classes (point 1-12), second part will be debugging the two things Alex detected.

First part is done and already on trunk, see my comments below. Second part will come soon, keep the ticket open.

Replying to Alex Bukreev <bucreev@…>:

1. class “error” – base class, exception in this class are not generated (i suggest to declare its constructor as ‘protected’).

-> Currently class error is thrown from several places within the library.

I would suggest to introduce a new exception class for them, something like miscellaneous_error and then make constructor protected

2. class “invalid_syntax” – there is a “msg” parameter of type ‘string’ which is specified in constructor, it is necessary to swap its parameter type to ‘enum’ with storing of its value (like it’s done in class “invalid_command_line_syntax”).

-> DONE. Moved the "kind" to base class invalid_syntax.

3. class “unknown_option” – ok.

4. class “ambiguous_option” - – there is a “name” parameter specified in constructor. Parameter value must be stored.

-> DONE

5. class "multiple_values" - there is a “what” parameter specified in constructor which is always equal to "multiple_values" parameter.

• remove “what” parameter from constructor.

-> DONE

6. class "multiple_occurrences" - there is a “what” parameter specified in constructor which is always equal to "multiple_values" parameter.

• remove “what” parameter from constructor.

-> DONE

7. class "validation_error - there is a “what” parameter of type ‘string’ specified in constructor. It is necessary to swap its parameter type to ‘enum’ with storing of its value.

-> DONE, introduce a kind_t and related function to derive message from

kind.

8. class "invalid_option_value" - value of parameter "bad_value" is not stored.

-> stored now in base class: validation_error

9. class "too_many_positional_options_error" – if this exception may occur only because of programmer(not user) mistake, then it’s ok.

-> comment added, removed what parameter

10. class "too_few_positional_options_error" - if this exception may occur only because of programmer(not user) mistake, then it’s ok.

-> class is not used - removed

11. class "invalid_command_line_syntax" – ok

-> see 2., moved kind_t to base class

12. class "invalid_command_line_style" - if this exception may occur only because of programmer(not user) mistake, then it’s ok.

-> Comment added

In general there are still some TODO's in the code. Storing non-trivial values in exception class might be a bad idea, because their constructors can throw. But I think these things can be kept in for now and marked with a TODO, cleaning up those would be another ticket.

With "part-2" I'll also update the test case and add some more checks.

Please comment! Reviewing is welcome!

[Sascha Ochsenknecht]

Replying to Alex Bukreev <bucreev@…>:

While testing of “error” classes were detected the following: 1.Source one:

options_description desc; desc.add_options()

("cfgfile,c", value<string>()->multitoken(), "the config file") ("output,c", value<string>(), "the output file") ("output,o", value<string>(), "the output file")

;

const char* cmdline[] = {"program", "-c", "file", "-o", "anotherfile"};

Just checkin a solution which throws ambiguous_option exception. This is thrown during parsing of commandline, not during add_options().

Cheers, Sascha

[Sascha Ochsenknecht]

(In [58184]) Better detection of missing values on command line, Fixes #3423

[anonymous]

Replying to Alex Bukreev <bucreev@…>:

2.Source two:

desc.add_options()

("cfgfile,c", value<string>()->multitoken(), "the config file") ("output,o", value<string>(), "the output file")

;

const char* cmdline[] = {"program", "-c", "-o", "anotherfile"};

Wrong diagnostic: "in option 'cfgfile': multiple values not allowed"

It detects "-o" as an option (not a value for "-c"). It now throws invalid_command_line_syntax(invalid_syntax::missing_parameter) for cfgfile. If "-o" should be recognized as value of "-c" it must be quoted.

Reviews are welcome :-). If set status to "closed" but if there are still issues feel free to re-open.

Thanks, SAscha

[stimming@…]

Unfortunately in [58138] and [58184] you changed the previously existing member variable invalid_syntax::tokens into an accessor function invalid_syntax::tokens(). This breaks existing code that wanted to print a helpful error message to the user, and with a rather weird compiler error message. Couldn't you introduce a more graceful change of interface here? Like, at least introducing the accessor function with a different name, so that the developer more easily understands you have removed the public member? Thanks!

[Vladimir Prus]

Would changing this for 1.43 actually help you, given that 1.42 is already out in the wild with this incompatibility?

[stimming@…]

Well, for me it doesn't change anything now that I've encountered this issue and added an #if BOOST_VERSION... workaround. But for other users it would probably be preferrable to solve this differently - maybe add a invalid_syntax::get_tokens() accessor, and marking the invalid_syntax::tokens() accessor as deprecated?

It's not an uncommon thing to fix some accidental backward incompatiblity in the next boost version, so yes, it would still be an improvement.