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

implement URLPattern #785

Open
wants to merge 114 commits into
base: main
Choose a base branch
from
Open

implement URLPattern #785

wants to merge 114 commits into from

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Nov 30, 2024

This implements URLPattern from scratch. I'm adding TODO: Optimization opportunity notes in places I think an optimization can be done. The goal is to merge this once there is no TODO: Implement this comments.

TODOs

@anonrig anonrig requested review from lemire and jasnell November 30, 2024 17:53
@anonrig anonrig force-pushed the yagiz/add-url-pattern branch from 89f46f0 to 02492e1 Compare November 30, 2024 18:18
@anonrig anonrig mentioned this pull request Nov 30, 2024
9 tasks
@anonrig anonrig force-pushed the yagiz/add-url-pattern branch from 0ccd6d3 to d2cd4f8 Compare November 30, 2024 22:07
@lemire
Copy link
Member

lemire commented Dec 3, 2024

@anonrig Just to confirm... is this the spec? https://urlpattern.spec.whatwg.org

@anonrig
Copy link
Member Author

anonrig commented Dec 3, 2024

@anonrig Just to confirm... is this the spec? https://urlpattern.spec.whatwg.org

Yes that's correct @lemire

@anonrig anonrig force-pushed the yagiz/add-url-pattern branch 3 times, most recently from e51503f to 524cb73 Compare December 4, 2024 01:29
@anonrig anonrig changed the title implement URLPattern skeleton implement URLPattern Dec 4, 2024
include/ada/url_pattern.h Outdated Show resolved Hide resolved
include/ada/url_pattern.h Outdated Show resolved Hide resolved
include/ada/url_pattern.h Outdated Show resolved Hide resolved
@anonrig anonrig force-pushed the yagiz/add-url-pattern branch from 1a98cc9 to 18ef1a6 Compare December 4, 2024 16:42
@anonrig anonrig requested a review from jasnell December 4, 2024 16:42
@lemire
Copy link
Member

lemire commented Dec 7, 2024

I don't particular mind, but please be aware that you are changing the coding convention. The C++ standard library uses all-lowercase with underscores names. You are using Camel case. If you merge this, it will mean that within the same library, we mix two conventions.

@anonrig anonrig force-pushed the yagiz/add-url-pattern branch 3 times, most recently from 380d57e to 880e260 Compare December 12, 2024 18:06
@anonrig anonrig force-pushed the yagiz/add-url-pattern branch 5 times, most recently from 51690eb to 010bc69 Compare December 12, 2024 21:11
@anonrig
Copy link
Member Author

anonrig commented Dec 12, 2024

@jasnell @lemire would you mind taking a look? we're getting close to the finish line (although the test runner is missing).

@anonrig anonrig force-pushed the yagiz/add-url-pattern branch from b893367 to 7393441 Compare December 26, 2024 00:29
anonrig and others added 6 commits December 25, 2024 19:52
* adding some optional logging of the parse results

* guarding

* mark it as used

* fixed ECMAScript qualification

---------

Co-authored-by: Daniel Lemire <[email protected]>
@lemire
Copy link
Member

lemire commented Dec 28, 2024

@anonrig The last valid commit is 41ec170 (5 days ago).

Everything afterward is badly broken in GCC (debug or with sanitizer).

std::regex_constants::syntax_option_type flags{};
std::regex regexp{};
std::regex_constants::syntax_option_type flags = std::regex::ECMAScript;
std::regex regexp{"*"};

Choose a reason for hiding this comment

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

here I saw that wpt_urlpattern_tests.has_regexp_groups test passes when we use .* for ecmascript compatibility

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