-
Notifications
You must be signed in to change notification settings - Fork 1
Value parsing improvement #28
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…it can be used with more specializations of parse_value<T>()
…1", "false", "no", "n" and "0" case insensitively
…:filesystem::path
…clargs into rename-from_string-to-parse_value
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.
clang-tidy made some suggestions
8e966e1
to
4e35673
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In this development branch, the function
from_string
has been renamed toparse_value
to better convey the purpose of the function. This is especially important as it is this function that users of the library can use to support parsing their own value types from the command-line.Additionally, I have reworked the implementation of parsing integers and floating-point numbers and added support for parsing string_views and time units. Below are some more details for each type:
Parsing integers
Previosuly, integers were parsed using functions like
std::stoi
andstd::stol
. Though this is fine, it gets annoying using all the different versions of the function based on the integer type (int, long, unsigned short...). It is also generally less performant than the alternative we are using now:std::from_chars
. Instead of handling all integer types separately, I have specializedparse_value
for all types that satisfy the conceptstd::integral
and usestd::from_chars
to handle the actual parsing after some input validation. Another benefit is getting support for parsing values in hexadecimal and binary format basically for free.Parsing floating-point numbers
For floating-point types, we were previously using the floating-point equivalent of the integer ones;
std::stof
,std::stod
andstd::stold
. As with integer types, I have in this branch utilized
std::from_chars
, specializingparse_value
for all types that satisfy the conceptstd::floating_point
.String views
I decided to add support for parsing
std::string_view
. All it does it basically just return the input as the output, but it is an essential specialization as it allows users to specify string as the value type of an option without requiring to allocate an entirestd::string
. The once concern with this is that if users decide to manipulate the elements ofargv
after parsing it could lead to issues, as the string views point to the actual memory in the argv array. It is essential to document this behaviour to users and enfore they do not mess withargv
after parsing if they choose to use string view as a value type.Time units
Passing time points and durations is quite common in command-line interfaces so it is useful for the library to be able to parse this for the user automatically. In this branch, I have specialized
parse_value
for allstd::chrono::duration
type, meaning they can specify the value type to be for examplestd::chrono::seconds
, or more specialized types likestd::chrono::duration<double, std::milli>
.Error handling in parse_value
I decided to implement an exception class
ParseValueException
that is templated on the type that is parsed. This enables some centralizing some common error message logic, and printing the type of the type that the error occurs for. For example, trying to parse "asd" as anint
may produce the following error message:Similarly, trying to parse "asd" as the type
std::chrono::milliseconds
may produce the following error message:Since part of parsing a
std::chrono::milliseconds
involved parsing the underlying type, we end up with an error message that explains quite a lot of what went wrong: We tried to parse "asd" as type "milliseconds", this involved parsing it as a 64-bit signed integer, and that failed because the format was invalid.How Has This Been Tested?
The file tests/parse_value_tests.cpp contains a comprehensive test of tests (868 assertions in 117 test cases) for all specializations of
parse_value
.