Option parsing #142

wthrowe · 2017-08-03T21:18:20Z

I have made many changes from the original code. Some of the highlights:

Most of the documentation and error output has been rewritten, including more checks for incorrect input.
The parser-enforced value checks have been simplified: bounds checks on individual container elements are gone, fixed size vectors are gone (but could still be obtained using min size=max size), and all remaining constraints are shown in the help output.
Formatting checks are done on option help. (These existed before but were not run due to a bug.)
MAKE_CREATABLE_FROM_YAML macro for non-factory stuff as discussed elsewhere.
A "forwarding" form for MAKE_CREATABLE_FROM_YAML to allow parsing simple types without using full option parsing (e.g., parsing an old-style Point class as Center: [1, 2, 3] or an enum as Norm: L2).
Constructors called from Factory and MCFY now take an OptionContext parameter that can be passed to the PARSE_ERROR macro to include information about the input file location in an error message (generally for doing more complex checks on input values).
OptionsList in factory/MCFY classes has been renamed to options because it should be camel case and it isn't a list in MCFY forwarding.
Added `Options<...>::apply

I've put some examples of parsing error output at http://astro.cornell.edu/~wthrowe/parsing_errors (executable at http://astro.cornell.edu/~wthrowe/parsing_errors.cpp).

wthrowe · 2017-08-04T18:02:05Z

Builds are failing because yaml-cpp is too old. The latest release can't handle non-copyable types. We need something more recent than November 2016.

nilsdeppe · 2017-08-04T18:03:31Z

Okay, I can update the container we run on Travis

wthrowe · 2017-08-04T18:17:07Z

I've prepared a commit documenting the yaml-cpp version requirement, but I'll hold off pushing until it can trigger a Travis run with the update.

nilsdeppe · 2017-08-07T15:47:46Z

src/Options/Options.hpp

+/// parser, for use in printing errors.  A default-constructed
+/// OptionContext is printed as an empty string.  This struct is
+/// primarily used as an argument to PARSE_ERROR for reporting input
+/// file parsing errors.


Could you add a brief explanation of what YAML::Mark is used for or link to the yaml-cpp documentation? The yaml-cpp documentation is not exactly super complete or easy to navigate. If I understand correctly, YAML::Mark used to store the line and column in the file where a parsing error occurred?

nilsdeppe · 2017-08-07T20:36:59Z

src/Utilities/ConstantExpressions.hpp

@@ -168,13 +168,13 @@ inline constexpr auto make_array_from_list() {

 /// \ingroup ConstantExpressions
 /// \brief Compute the length of a const char* at compile time
-SPECTRE_ALWAYS_INLINE constexpr size_t cstring_length(const char* str) {


Can the place that caused the error use a std::string instead which actually supports these features?

nilsdeppe · 2017-08-07T20:39:31Z

src/Options/Options.hpp

+// See LICENSE.txt for details.
+
+/// \file
+/// Defined classes and functions that handle parsing of input parameters.


nilsdeppe · 2017-08-07T20:43:15Z

src/Options/Options.hpp

+
+  void append(const std::string& c) { context += c + ":\n"; }
+};
+inline std::ostream& operator<<(std::ostream& s, const OptionContext& c) {


nilsdeppe · 2017-08-07T20:45:27Z

src/Options/Options.hpp

+/// data and an OptionContext.
+class Option_t {
+ public:
+  Option_t() = delete;


add comment saying we don't want a default constructor

nilsdeppe · 2017-08-07T20:47:08Z

src/Options/Options.hpp

+
+  /// NOTE: This constructor overwrites the mark data in the supplied
+  /// context with the one from the node.
+  Option_t(YAML::Node node, OptionContext context = OptionContext())


nilsdeppe · 2017-08-07T21:35:09Z

src/Options/OptionsDetails.hpp

+
+/// Holds details of the implementation of Options
+namespace Options_details {
+template <typename S, typename U = void>


U = void -> = cpp17::void_t<>

nilsdeppe · 2017-08-07T21:39:55Z

src/Options/OptionsDetails.hpp

+  template <typename T>
+  void operator()(tmpl::type_<T> /*meta*/) {
+    std::ostringstream ss;
+    ss << "  " << std::setw(max_label_size_) << std::left << T::label


look at setw things

nilsdeppe · 2017-08-07T21:42:44Z

src/Options/OptionsDetails.hpp

+       << std::setw(0) << T::help << "\n\n";
+    value += ss.str();
+  }
+  value_type value{};


blank line above

nilsdeppe · 2017-08-07T21:45:08Z

src/Options/OptionsDetails.hpp

+  }
+}
+
+template <typename T>


move to TypeTraits.hpp

nilsdeppe · 2017-08-07T21:52:19Z

tests/Unit/Options/Test_Options.cpp

+}  // namespace
+
+TEST_CASE("Unit.Options.Defaulted.specified", "[Unit][Options]") {
+  const std::string yaml = "Defaulted: 20";


inline short strings

nilsdeppe · 2017-08-07T22:08:54Z

src/Options/Factory.hpp

+///
+/// To use a factory, the base class (here `BaseClass`) should inherit
+/// from Factory<BaseClass> and define a static method \code
+///  static std::string class_id() { return "BaseClass"; }


Could experiment with using pretty_type to replace class_id()

nilsdeppe · 2017-08-07T22:10:52Z

src/Options/Factory.hpp

+/// from Factory<BaseClass> and define a static method \code
+///  static std::string class_id() { return "BaseClass"; }
+/// \endcode
+/// and a type \code


\code to next line

nilsdeppe · 2017-08-07T22:11:09Z

src/Options/Factory.hpp

+/// input file.
+///
+/// To use a factory, the base class (here `BaseClass`) should inherit
+/// from Factory<BaseClass> and define a static method \code


\code on next line

nilsdeppe · 2017-08-07T22:12:27Z

src/Options/Factory.hpp

+///    class method shown above.
+/// 2) define static OptionString_t help containing class-specfic help
+///    text
+/// 3) define a type options as a typelist of option structs required


define a type alias options

nilsdeppe · 2017-08-07T22:13:42Z

src/Options/Factory.hpp

+  static std::string help_derived();
+
+  template <
+      typename CreateLs,


CreateLs to CreateList

nilsdeppe · 2017-08-07T22:23:53Z

src/Options/Factory.hpp

+};
+
+template <typename BaseClass>
+template <typename CreateLs,


nilsdeppe · 2017-08-07T22:28:39Z

src/Options/Factory.hpp

+  derived_opts.Context().append("While creating type " + id);
+  auto derived =
+      create_derived<typename BaseClass::creatable_classes>(id, derived_opts);
+  if (derived) {


derived != nullptr for clarity

nilsdeppe · 2017-08-07T22:29:08Z

src/Options/Factory.hpp

+       << std::setw(name_col) << ""
+       << std::setw(help_col - name_col - 1) << T::class_id();
+    if (ss.str().size() >= help_col) {
+      ss << std::setw(0) << "\n" << std::setw(help_col - 1) << "";


Don't need to setw(0)

nilsdeppe · 2017-08-07T22:32:26Z

src/Options/Factory.hpp

+    return true;
+  }
+};
+}


// namespace YAML

nilsdeppe · 2017-08-07T22:39:24Z

tests/Unit/Options/Test_Factory.cpp

+  std::string arg_;
+};
+
+struct OptionsVecArg {


kidder · 2017-08-06T03:31:29Z

src/Options/Options.hpp

+  static_assert(cpp17::is_same_v<decltype(T::lower_bound()), typename T::type>,
+                "Lower bound is not of the same type as the option.");
+  static_assert(not cpp17::is_same_v<typename T::type, bool>,
+                "Cannot set an lower bound for a bool.");


wthrowe · 2017-08-10T20:40:00Z

The new commits fall into a few groups:

Minor fixes, largely based on above comments
Removing class_id and option labels in favor of using the name of the class/struct. Assuming we don't run into any issues with compiler/library support, I think the class_id changes are a clear win. The option label changes sometimes force you to name option structs in ways that may conflict with other names. This can be avoided by wrapping options in an additional struct, so I think we're still OK, but the change is not completely downside free.
Redesigning MAKE_CREATABLE_FROM_YAML, since in trying to write an example I found some serious problems with the previous version. (This does mean that T::options is always a typelist again, so it can be changed back to T::options_list if anyone cares.)

There was discussion of adding support for giving the option parser a small set of values allowed for an option. There is currently support for adding parsability to an enum (see example on MCFY) and for providing parsing errors from factory class constructors. The only additional feature that I think the parser could add would be displaying options in the help text, but we're trying to keep that short so I'm kind of leery of printing arbitrary lists of values there.

wthrowe · 2017-08-10T21:08:25Z

And looks like yaml-cpp is broken on the clang Travis setups.

wthrowe · 2017-08-10T21:28:03Z

Trying to clean up the doxygen warnings, I've hit two issues:

It looks like doxygen doesn't understand decltype(auto) return type.
One of the corrected \snippet lines is over 80 characters, and doxygen doesn't allow the line to be broken. I guess we just have to use short tag names in files with long names?

kidder · 2017-08-11T00:57:11Z

src/Options/Options.hpp

    context_.mark = node_.Mark();
  }

+  /// Convert to an object ot type `T`.


kidder · 2017-08-11T01:08:16Z

src/Options/Options.hpp

-  for (auto it = options.begin(); it != options.end(); ++it) {
-    const auto key = it->first.as<std::string>();
-    const auto value = it->second;
+  for (const auto& kv : node) {


while I've used kv for the range-based key-value pair, others ( @gsb76 , @osheamonn ) have voiced other opinions

kidder · 2017-08-11T01:20:36Z

src/Utilities/PrettyType.hpp

+ * \brief Extract the "base name" from a name, that is, the name
+ * without template parameters or scopes.
+ */
+std::string extract_base_name(std::string name);


I cannot think of an alternative at the moment, but when I first saw base_name I thought about the name of a base class...

kidder · 2017-08-11T01:26:52Z

src/Options/Factory.hpp

-///    returns a unique id for the derived class, similar to the base
-///    class method shown above.
-/// 2) define static OptionString_t help containing class-specfic help
+/// 1) define static OptionString_t help containing class-specfic help


doxygen does have a syntax for enumerated list (having to relabel the comments made me think of this...)

kidder · 2017-08-11T01:36:40Z

src/Options/MakeCreatableFromYaml.hpp

-/// the `options` and an OptionContext.
+/// A class to be created must be default constructible and move
+/// assignable.  If create_from_yaml is not specialized for the class
+/// it must additionally declare a tnpl::list of Options-style option


tnpl -> tmpl

wthrowe · 2017-08-11T03:01:09Z

Speculation on the Travis yaml-cpp problem: using a libstdc++-linked yaml-cpp for a libc++ build.

nilsdeppe · 2017-08-11T03:25:19Z

Yes, that is what it is.

kidder · 2017-08-23T20:55:52Z

src/Options/Options.hpp

+  /// Parse an Option_t to obtain options and their values.
+  void parse(const Option_t& options);
+
+  /// Parse an file containing options


kidder · 2017-08-23T20:59:33Z

src/Options/Options.hpp

+  //@}
+
+  //@{
+  /// Check for an iterable type that the size is above the upper bound


above -> below

kidder · 2017-08-23T21:01:53Z

src/Options/Options.hpp

+  /// If the options has a lower bound, check it is satisfied.
+  ///
+  /// Note: Lower bounds are >=, not just >.
+  /// \tparam T the option "type"


"type" -> something clearer

kidder · 2017-08-23T21:02:06Z

src/Options/Options.hpp

+  /// If the options has a upper bound, check it is satisfied.
+  ///
+  /// Note: Upper bounds are <=, not just <.
+  /// \tparam T the option "type"


kidder · 2017-08-23T21:10:06Z

src/Options/Options.hpp

+
+// \cond
+// Doxygen is confused by decltype(auto)
+template <typename OptionsList>


OptionsList -> OptionList

kidder · 2017-08-23T21:39:13Z

tests/Unit/Options/Test_Options.cpp

+};
+/// [options_example_scalar_struct]
+}  // namespace
+


test that a out-of-bounds default fails

nilsdeppe

squash

nilsdeppe · 2017-09-01T18:49:04Z

src/Options/Options.hpp

@@ -232,7 +232,10 @@ class Options {
  void parse(const YAML::Node& node);

  //@{
-  /// Check for an iterable type that the size is above the lower bound
+  /// Check for an that the size is not smaller than the lower bound


Check that the....

nilsdeppe · 2017-09-01T18:49:16Z

src/Options/Options.hpp

@@ -247,7 +250,10 @@ class Options {
  //@}

  //@{
-  /// Check for an iterable type that the size is below the upper bound
+  /// Check for an that the size is not larger than the upper bound


nilsdeppe · 2017-09-01T18:50:25Z

src/Options/Options.hpp

@@ -263,6 +269,8 @@ class Options {

  //@{
  /// Returns the default value or errors if there is no default.
+  ///
+  /// \tparam T the option struct
  template <typename T,
            std::enable_if_t<Options_details::has_default<T>::value>* = nullptr>


Change all to Requires<...> = nullptr

kidder · 2017-09-04T19:56:16Z

Will, can you rebase on the latest commit that attempted to fix Travis stuff

nilsdeppe · 2017-09-05T19:39:32Z

I'll look at what the heck Travis is doing

nilsdeppe · 2017-09-07T13:37:41Z

Could you rebase to get the new Travis things that uses libstdc++ with Clang?

nilsdeppe · 2017-09-08T12:44:34Z

@wthrowe could you try applying the changes in this commit nilsdeppe@0dfcc15 ?

codecov-io · 2017-09-08T22:42:05Z

Codecov Report

Merging #142 into develop will decrease coverage by 0.34%.
The diff coverage is 93.95%.

@@             Coverage Diff             @@
##           develop     #142      +/-   ##
===========================================
- Coverage    98.93%   98.59%   -0.35%     
===========================================
  Files          110      115       +5     
  Lines         3766     4047     +281     
===========================================
+ Hits          3726     3990     +264     
- Misses          40       57      +17

Impacted Files	Coverage Δ
src/Utilities/PrettyType.cpp	`100% <100%> (ø)`
src/Utilities/PrettyType.hpp	`100% <100%> (ø)`	⬆️
src/Options/MakeCreatableFromYaml.hpp	`100% <100%> (ø)`
src/Options/Options.hpp	`91.89% <91.89%> (ø)`
src/Options/Factory.hpp	`94.54% <94.54%> (ø)`
src/Options/OptionsDetails.hpp	`96.77% <96.77%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89f4671...b660505. Read the comment docs.

nilsdeppe reviewed Aug 7, 2017

View reviewed changes

nilsdeppe requested changes Aug 7, 2017

View reviewed changes

kidder requested changes Aug 8, 2017

View reviewed changes

wthrowe force-pushed the options branch from d9a841c to be1879b Compare August 10, 2017 20:39

kidder requested changes Aug 11, 2017

View reviewed changes

kidder requested changes Aug 23, 2017

View reviewed changes

kidder mentioned this pull request Sep 1, 2017

port options #169

Closed

nilsdeppe requested changes Sep 1, 2017

View reviewed changes

wthrowe force-pushed the options branch from b29a7c8 to 4d47810 Compare September 1, 2017 20:47

wthrowe force-pushed the options branch from 4d47810 to 4baea05 Compare September 5, 2017 18:49

wthrowe added 7 commits September 7, 2017 14:09

Add pretty_type::base_name function

5eea948

Add is_std_array_of_size type trait

be89285

Add OptionParsing doxygen group

8915cf9

Add Options class

e0eae8c

Add Factory class

e5e20d6

Add MAKE_CREATABLE_FROM_YAML macro

bd333cc

Document required yaml-cpp version

c2fe68a

wthrowe force-pushed the options branch from 4baea05 to c2fe68a Compare September 7, 2017 18:11

Output test failures during coverage generation

d984864

Have Travis load external packages when running coverage

b660505

nilsdeppe approved these changes Sep 8, 2017

View reviewed changes

kidder approved these changes Sep 8, 2017

View reviewed changes

kidder merged commit e69a135 into sxs-collaboration:develop Sep 8, 2017

nilsdeppe mentioned this pull request Sep 23, 2017

Options: specify overall help-string near OptionsList #19

Closed

Option parsing #142

Option parsing #142

Conversation

wthrowe commented Aug 3, 2017

wthrowe commented Aug 4, 2017

nilsdeppe commented Aug 4, 2017

wthrowe commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wthrowe commented Aug 10, 2017

wthrowe commented Aug 10, 2017

wthrowe commented Aug 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wthrowe commented Aug 11, 2017

nilsdeppe commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsdeppe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidder commented Sep 4, 2017

nilsdeppe commented Sep 5, 2017

nilsdeppe commented Sep 7, 2017

nilsdeppe commented Sep 8, 2017

codecov-io commented Sep 8, 2017 • edited Loading

Codecov Report

codecov-io commented Sep 8, 2017 •

edited

Loading