Boost C++ Libraries: Ticket #3423: Diagnostic of errors.

cc set

samjmill@… — Mon, 09 Nov 2009 18:22:57 GMT

cc samjmill@… added

added myself

attachment set

Wed, 18 Nov 2009 06:55:57 GMT

attachment → ticket3423.patch

patch

cc changed

Wed, 18 Nov 2009 07:00:02 GMT

cc s.ochsenknecht@… added

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 — Wed, 18 Nov 2009 13:32:52 GMT

Sascha,

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

You seem to add an empty line to the 'error' class definition. I've removed it.
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.

status changed; resolution set

Vladimir Prus — Wed, 18 Nov 2009 13:35:17 GMT

status new → closed
resolution → fixed

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

Fixes #3423. Patch from Sascha Ochsenknecht.

status changed; resolution deleted

Tue, 24 Nov 2009 05:55:38 GMT

status closed → reopened
resolution fixed

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

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

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”).

class “unknown_option” – ok.

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

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

remove “what” parameter from constructor.

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

remove “what” parameter from constructor.

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.

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

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

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

class "invalid_command_line_syntax" – ok

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"

owner, status, milestone changed

Sascha Ochsenknecht — Thu, 26 Nov 2009 10:52:00 GMT

owner changed from Vladimir Prus to Sascha Ochsenknecht
status reopened → new
milestone Boost 1.41.0 → Boost 1.42.0

status changed

Sascha Ochsenknecht — Thu, 26 Nov 2009 10:54:08 GMT

status new → assigned

Sascha Ochsenknecht — Fri, 04 Dec 2009 13:39:23 GMT

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@…>:

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

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.

class “unknown_option” – ok.

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

-> DONE

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

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

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.

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

-> stored now in base class: validation_error

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

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

class "invalid_command_line_syntax" – ok

-> see 2., moved kind_t to base class

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 — Sat, 05 Dec 2009 08:08:56 GMT

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

status changed; resolution set

Sascha Ochsenknecht — Sun, 06 Dec 2009 09:52:57 GMT

status assigned → closed
resolution → fixed

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

anonymous — Sun, 06 Dec 2009 10:00:23 GMT

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@… — Tue, 02 Mar 2010 16:45:59 GMT

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!

status changed; resolution deleted

Vladimir Prus — Sat, 06 Mar 2010 08:46:50 GMT

status closed → reopened
resolution fixed

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

stimming@… — Sat, 06 Mar 2010 10:10:32 GMT

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.