-
Notifications
You must be signed in to change notification settings - Fork 287
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
[regex-ripout] part 2/n #431
Conversation
also rename reformat_version -> format_version_for_nugetref
src/vcpkg/tools.cpp
Outdated
@@ -25,25 +25,57 @@ namespace vcpkg | |||
std::string sha512; | |||
}; | |||
|
|||
static Optional<std::array<int, 3>> parse_version_string(const std::string& version_as_string) | |||
// /\d+\.\d+(\.\d+)?/ | |||
static Optional<std::array<int, 3>> parse_version_string(StringView string_version) |
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.
While I have outstanding changes to tools.cpp, this change will not create issues for me.
src/vcpkg/versions.cpp
Outdated
} | ||
|
||
// /(\d+)(\.\d+|$)(\.\d+)?.*/ | ||
bool try_parse_external_dot_version(ParsedExternalVersion& out, StringView version) |
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.
This needs a test consisting of at least all the current values from vcpkgTools.xml
.
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.
This API is different in character than our other try_parse_Xxx functions in that they tend to return a structure (or write to a structure) that owns its data, while the lifetime of the data fed to this is tied to the caller. I'm a little worried of someone doing:
try_parse_external_dot_version(x, something_that_returns_std_string())
Would it make sense to make ParsedExternalVersion have strings rather than StringViews inside?
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.
I'd prefer to not do that, but I will if you insist. This is mostly an implementation detail function that's been pulled out into it's own thing.
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.
I think that's reasonable; do we want a new name like "try_extract" rather than "try_parse" to indicate this ownership difference? ("I think you're being paranoid Bill" is a reasonable answer ;))
Additionally, this includes a change to be more correct: &#x<hex>; is now considered a character reference.
also format
e05d570
to
5143398
Compare
5143398
to
7bf88dc
Compare
a08151e
to
9941c3f
Compare
7dd84dd
to
6d7d4ef
Compare
# Conflicts: # include/vcpkg/base/strings.h # src/vcpkg/commands.autocomplete.cpp # src/vcpkg/sourceparagraph.cpp
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.
This approval is conditional on removing the now unused <charconv>
include; everything else are nitpicks.
Thanks for adding tests :D |
00d54c8
to
399b13a
Compare
No description provided.