Modify

Opened 8 years ago

Last modified 8 years ago

#3423 reopened Feature Requests

Diagnostic of errors.

Reported by: Alex Bukreev <bucreev@…> Owned by: Sascha Ochsenknecht
Milestone: Boost 1.42.0 Component: program_options
Version: Boost 1.40.0 Severity: Problem
Keywords: Cc: samjmill@…, s.ochsenknecht@…

Description

  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.

Attachments (1)

ticket3423.patch (9.3 KB) - added by Sascha Ochsenknecht <s.ochsenknecht@…> 8 years ago.
patch

Download all attachments as: .zip

Change History (15)

comment:1 Changed 8 years ago by samjmill@…

Cc: samjmill@… added

added myself

Changed 8 years ago by Sascha Ochsenknecht <s.ochsenknecht@…>

Attachment: ticket3423.patch added

patch

comment:2 Changed 8 years ago by Sascha Ochsenknecht <s.ochsenknecht@…>

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.

comment:3 Changed 8 years ago by 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.

comment:4 Changed 8 years ago by Vladimir Prus

Resolution: fixed
Status: newclosed

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

Fixes #3423. Patch from Sascha Ochsenknecht.

comment:5 Changed 8 years ago by Alex Bukreev <bucreev@…>

Resolution: fixed
Status: closedreopened

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’).
  1. 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”).
  1. class “unknown_option” – ok.
  1. class “ambiguous_option” - – there is a “name” parameter specified in constructor. Parameter value must be stored.
  1. class "multiple_values" - there is a “what” parameter specified in constructor which is always equal to "multiple_values" parameter.
  • remove “what” parameter from constructor.
  1. class "multiple_occurrences" - there is a “what” parameter specified in constructor which is always equal to "multiple_values" parameter.
  • remove “what” parameter from constructor.
  1. 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.
  1. class "invalid_option_value" - value of parameter "bad_value" is not stored.
  1. class "too_many_positional_options_error" – if this exception may occur only because of programmer(not user) mistake, then it’s ok.
  1. class "too_few_positional_options_error" - if this exception may occur only because of programmer(not user) mistake, then it’s ok.
  1. class "invalid_command_line_syntax" – ok
  1. 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"

comment:6 Changed 8 years ago by Sascha Ochsenknecht

Milestone: Boost 1.41.0Boost 1.42.0
Owner: changed from Vladimir Prus to Sascha Ochsenknecht
Status: reopenednew

comment:7 Changed 8 years ago by Sascha Ochsenknecht

Status: newassigned

comment:8 in reply to:  5 Changed 8 years ago by 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

  1. 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.

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

-> DONE

  1. 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

  1. 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

  1. 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.

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

-> stored now in base class: validation_error

  1. 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

  1. 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

  1. class "invalid_command_line_syntax" – ok

-> see 2., moved kind_t to base class

  1. 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!

comment:9 in reply to:  5 Changed 8 years ago by 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

comment:10 Changed 8 years ago by Sascha Ochsenknecht

Resolution: fixed
Status: assignedclosed

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

comment:11 in reply to:  5 Changed 8 years ago by 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

comment:12 Changed 8 years ago by 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!

comment:13 Changed 8 years ago by Vladimir Prus

Resolution: fixed
Status: closedreopened

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

comment:14 Changed 8 years ago by 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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as reopened The owner will remain Sascha Ochsenknecht.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.