Skip to content
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

Start removing std::regex #427

Merged
merged 10 commits into from
Mar 11, 2022
Merged

Conversation

strega-nil
Copy link
Contributor

No description provided.

@strega-nil strega-nil changed the title Remove std::regex Start removing std::regex Mar 10, 2022
// 0 1 2 3 4 5 6 7 8 9 10
if (version.size() < 10) return false;
auto first = version.begin();
if (!P::is_ascii_digit(*first++)) return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self/others: it would be really nice to not have yet another parser for this vs the one in DateVersion. No change requested.

// - v?<X>.<Y><whatever> -> <X>.<Y>.0-vcpkg<abitag>
// - v?<X>.<Y>.<Z><whatever> -> <X>.<Y>.<Z>-vcpkg<abitag>
// - anything else -> 0.0.0-vcpkg<abitag>
std::string reformat_version(StringView version, StringView abi_tag);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
std::string reformat_version(StringView version, StringView abi_tag);
std::string add_abitag_version_suffix(StringView version, StringView abi_tag);

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it doesn't look like vcpkg::reformat_version is exercised unless a nuget binary or asset cache is used, I would like to see some unit tests added for that (and renamed as commented before). Other than that LGTM!

@strega-nil-ms
Copy link
Contributor

@BillyONeal there are unit tests for it already (hence why I'm confident that the new code is equivalent to the old code :P)

@BillyONeal
Copy link
Member

@BillyONeal there are unit tests for it already (hence why I'm confident that the new code is equivalent to the old code :P)

"cool beans"

@strega-nil-ms strega-nil-ms merged commit 674ea53 into microsoft:main Mar 11, 2022
@strega-nil-ms strega-nil-ms deleted the remove-regex branch March 11, 2022 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants