Merged.

Sorry, I got interrupted by a call while I was going through the pull request. I now finished looking, everything looks good to me.

I didn't look at the changes to OptionParser::add_enum_option because of the way that diff is shown on bitbucket, but  I don't care much about the implementation details of this function anyway; I care much more about the interfaces. So from my side this can be merged. Many thanks!

Malte, I saw you left one comment. Anything else or should we merge this?

OK, this can be merged from my point of view.

I did the minimalistic change :-) ShrinkFH::HighLow was already public within the class and I didn't want to move it out of the class just to have a smaller diff. All other enum classes were private before and I preferred to move them outside the class because I think that this is what we do in most cases (maybe all except of ShrinkFH::HighLow).

The code looks good to me. Out of curiosity: why did you make some enum classes visible on the top level and not others (e.g., ShrinkFH::HighLow)?

I updated the PR and the title of this issue accordingly. All tests ran through successfully. Do you want to have another quick look?

Agreed. I don't think we can get rid of all special handling of enums right away, but I would be in favour of getting rid of the special cases that are not needed.

Current C++ does not offer a way to access the string names for enum entries in a way that would allow producing the strings we need automatically. Some people define enums indirectly with macros or use code generators to work around this, but I wouldn't advocate that.

The situation may change if the current reflection/metaclasses proposals are accepted, but this will be after C++20.

I think OptionParser::add_enum_option() must still be a separate method, but it should be templated by the enum type. Then we can store the enum instead of an int in the Options object.

We could indeed just store the enum directly in the options storage. However, we a quite involved mechanism for adding enum options to a parser: we provide a vector of strings, where the values *should* correspond to the names of the enum values. However, only the position within the vector matters. When parsing the option, it is processed as a string to find out which name/position of the enum is meant. Using the regular Parse::add_option method does not work out of the box because the given strings cannot be compared against enum values. I don't know if there is a way to obtain a string representation of enum values or if there is any other solution of mapping given strings to enum values.

I think it is because C-style enums weren't enough of a real type for a regular template overload. But I think that this has changed with C++11/"enum class". Not sure.

I don't know. I always assumed there was a reason that the option parser treats enums specially. But I'll investigate.

If we now want to work with actual enum types, why a special set_enum method at all, rather than the vanilla templated set method? Also, why convert to int inside the Options storage and then compare back? Does our internal storage type not support enums, or is there another reason? (I did not try this out.)

Yes, added. Unless I hear objections, I'll proceed to merge.

Looks good to me. I think this warrants a changelog entry.

Pull request: https://bitbucket.org/SilvanS/fd-dev/pull-requests/56/issue962/diff

We currently have int Options::get_enum(key) which requires the user to cast the result to their enum. I think that the method should better read T Options::get_enum(key) so that the method itself can do the cast. Then the user doesn't need to know that enums are internally stored as ints. Also, to add enums to an Options object, we should have a method Options::set_enum(key, T) analogously to the usual Options::set(key, T) method.