#1279 Implement regex lookaround

Open
opened 1 year ago by wolfbeast · 7 comments
wolfbeast commented 1 year ago (Migrated from github.com)

Our irregexp has lookahead functionality but not (negative) lookbehind (special case regex).

Some websites would prefer to be able to use this, so it’d be a good enhancement to implement lookbehind in our JS regex module.
(now if only the old implementation was still used this would be trivial as it was a lot more maintainable...)

Our irregexp has lookahead functionality but not (negative) lookbehind (special case regex). Some websites would prefer to be able to use this, so it'd be a good enhancement to implement lookbehind in our JS regex module. (now if only the old implementation was still used this would be trivial as it was a lot more maintainable...)
wolfbeast commented 1 year ago (Migrated from github.com)
Owner

Blocks #1282

Blocks #1282
wolfbeast commented 1 year ago (Migrated from github.com)
Owner

Resolved in ce0dd36a78

Resolved in ce0dd36a78814c59950fde6c19413c1f7ea85ee1
g4jc commented 1 year ago (Migrated from github.com)
Owner

Getting build failures on trunk after this merge.

.../js/src/builtin/TestingFunctions.cpp:3906:19: error: ‘RegExpLookahead’ is not a member of ‘js::irregexp’

Fix appears trivial, assuming this is all that is needed to resolve this bug.

Possible patch:

diff --git a/js/src/builtin/TestingFunctions.cpp b/js/src/builtin/TestingFunctions.cpp
index 4363c7aed..072a72b75 100644
--- a/js/src/builtin/TestingFunctions.cpp
+++ b/js/src/builtin/TestingFunctions.cpp
@@ -3900,10 +3900,10 @@ ConvertRegExpTreeToObject(JSContext* cx, irregexp::RegExpTree* tree)
             return nullptr;
         return obj;
     }
-    if (tree->IsLookahead()) {
-        if (!StringProp(cx, obj, "type", "Lookahead"))
+    if (tree->IsLookaround()) {
+        if (!StringProp(cx, obj, "type", "Lookaround"))
             return nullptr;
-        irregexp::RegExpLookahead* t = tree->AsLookahead();
+        irregexp::RegExpLookaround* t = tree->AsLookaround();
         if (!BooleanProp(cx, obj, "is_positive", t->is_positive()))
             return nullptr;
         if (!TreeProp(cx, obj, "body", t->body()))

Getting build failures on trunk after this merge. `.../js/src/builtin/TestingFunctions.cpp:3906:19: error: ‘RegExpLookahead’ is not a member of ‘js::irregexp’` Fix appears trivial, assuming this is all that is needed to resolve this bug. Possible patch: ``` diff --git a/js/src/builtin/TestingFunctions.cpp b/js/src/builtin/TestingFunctions.cpp index 4363c7aed..072a72b75 100644 --- a/js/src/builtin/TestingFunctions.cpp +++ b/js/src/builtin/TestingFunctions.cpp @@ -3900,10 +3900,10 @@ ConvertRegExpTreeToObject(JSContext* cx, irregexp::RegExpTree* tree) return nullptr; return obj; } - if (tree->IsLookahead()) { - if (!StringProp(cx, obj, "type", "Lookahead")) + if (tree->IsLookaround()) { + if (!StringProp(cx, obj, "type", "Lookaround")) return nullptr; - irregexp::RegExpLookahead* t = tree->AsLookahead(); + irregexp::RegExpLookaround* t = tree->AsLookaround(); if (!BooleanProp(cx, obj, "is_positive", t->is_positive())) return nullptr; if (!TreeProp(cx, obj, "body", t->body())) ```
wolfbeast commented 1 year ago (Migrated from github.com)
Owner

Please make a PR for this. Fix is correct.

~Not sure why this didn’t pop up during my development?~ Ah, debug builds, OK.

Please make a PR for this. Fix is correct. ~Not sure why this didn't pop up during my development?~ Ah, debug builds, OK.
wolfbeast commented 1 year ago (Migrated from github.com)
Owner

Oh, I see now it was already fixed through a PR. Next time, please make sure a PR to fix an issue or fallout from it mentions it in its description also, so it gets linked in the issue at hand. Or mention in the issue itself with a follow-up comment that it was fixed (referencing the commit), so it’s clear no further action is needed.

Oh, I see now it was already fixed through a PR. Next time, _please_ make sure a PR to fix an issue or fallout from it mentions it in its description also, so it gets linked in the issue at hand. Or mention in the issue itself with a follow-up comment that it was fixed (referencing the commit), so it's clear no further action is needed.
wolfbeast commented 1 year ago (Migrated from github.com)
Owner

Backed out for causing #1362

Backed out for causing #1362
Moonchild commented 6 months ago
Owner

Superseded by #1675

Superseded by #1675
Sign in to join this conversation.
No Milestone
No Assignees
1 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.