-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FIX] parsing containers of enums not working #2381
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2381 +/- ##
=======================================
Coverage 98.21% 98.21%
=======================================
Files 265 265
Lines 10806 10820 +14
=======================================
+ Hits 10613 10627 +14
Misses 193 193
Continue to review full report at Codecov.
|
a94ac59
to
c6830d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, for the fast help!
LGFM :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but can we generalise it ?
requires input_stream_over<std::istringstream, typename container_option_t::value_type> || | ||
named_enumeration<typename container_option_t::value_type> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requires input_stream_over<std::istringstream, typename container_option_t::value_type> || | |
named_enumeration<typename container_option_t::value_type> | |
requires requires (std::ranges::value_type<container_option_t> & container_value, std::string const & in) | |
{ | |
{ parse_option_value(container_value, in) } -> std::same_as<option_parse_result>; | |
} |
wouldn't that be more general? If it is a container, try to defer to its value_type (that's exactly what the function does). The only problem would be container that have themselves as value_type xD
Since we also have overloads for bool and other things, I guess std::vector<bool>
does not work right now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good catch, std::vector<bool>
did not work. The concept was hard to write... tldr: you need an instance of the class if you want to test member functions, but the compiler does not necessarily tell you :)
I opted to use container_option_t::value_type
instead of the ranges thing:
- Doesn't seem to be currently included (less includes :) )
- It currently requires
sequence_container
, which requiresvalue_type
to be defined
If this is good, you can squash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
In #1196, we forgot to allow for, e.g.
std::vector<foo::bar>
, wherefoo::bar
is an enum.