Modify

Ticket #3423 (reopened Feature Requests)

Opened 5 years ago

Last modified 4 years ago

Diagnostic of errors.

Reported by: Alex Bukreev <bucreev@…> Owned by: s_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

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

Change History

comment:1 Changed 4 years ago by samjmill@…

  • Cc samjmill@… added

added myself

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

patch

comment:2 Changed 4 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 4 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 4 years ago by vladimir_prus

  • Status changed from new to closed
  • Resolution set to fixed

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

Fixes #3423. Patch from Sascha Ochsenknecht.

comment:5 follow-ups: ↓ 8 ↓ 9 ↓ 11 Changed 4 years ago by Alex Bukreev <bucreev@…>

  • Status changed from closed to reopened
  • Resolution fixed deleted

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 4 years ago by s_ochsenknecht

  • Owner changed from vladimir_prus to s_ochsenknecht
  • Status changed from reopened to new
  • Milestone changed from Boost 1.41.0 to Boost 1.42.0

comment:7 Changed 4 years ago by s_ochsenknecht

  • Status changed from new to assigned

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

  • Status changed from assigned to closed
  • Resolution set to fixed

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

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

  • Status changed from closed to reopened
  • Resolution fixed deleted

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

View

Add a comment

Modify Ticket

Change Properties
<Author field>
Action
as reopened
Author


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

 
Note: See TracTickets for help on using tickets.