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

[vcpkg] Prefer STREQUAL over MATCHES #23014

Merged

Conversation

SunBlack
Copy link
Contributor

@SunBlack SunBlack commented Feb 9, 2022

Avoid using CMake's well-known slow regex engine if it is not necessary

Used the regex if.*MATCHES "\^[\w\s]*\$" to find all occurrences.

@LilyWangLL LilyWangLL changed the title Prefer STREQUAL over MATCHES [vcpkg] Prefer STREQUAL over MATCHES Feb 10, 2022
@LilyWangLL LilyWangLL added the category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed label Feb 10, 2022
@LilyWangLL LilyWangLL requested a review from JackBoosY February 10, 2022 02:18
@JackBoosY
Copy link
Contributor

JackBoosY commented Feb 10, 2022

I don't think we're using regex because it's too slow.
cc @strega-nil-ms @ras0219-msft

Copy link
Contributor

@JackBoosY JackBoosY 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's full keyword regex. I think that's correct.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Feb 15, 2022
@BillyONeal BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Feb 16, 2022
@BillyONeal
Copy link
Member

These changes are all improvements, but not worth forcing everyone to rebuild the world over, as an edit to ports.cmake will do. As a result, we have tagged this with "world-rebuild" to potentially roll up into other changes that would affect that file.

However, the changes to buildsystems/vcpkg.cmake will not cause a world rebuild, so if you want to create a second PR with only those changes because you want those effects early, that would be OK. (Of course, given that this is just a code cleanup I doubt it's worth it to you)

@BillyONeal
Copy link
Member

BillyONeal commented Feb 16, 2022

Sorry, I've just been informed the correct tag is "next-rollup" rather than "world-rebuild"; the latter is about PRs in progress while this PR is otherwise ready-to-merge.

@ras0219-msft ras0219-msft merged commit 6a66e95 into microsoft:master May 24, 2022
@ras0219-msft
Copy link
Contributor

LGTM, thanks for the improvement!

@SunBlack SunBlack deleted the prefer_STREQUAL_over_MATCHES branch May 25, 2022 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants