Implement String.prototype.matchAll and related
#1731
Conversation
|
@andreabergia Great ❤️ the architecture tests are failing with jdk 21 - will have a look later if you like |
Noticed. There's two ways to make this pass:
I have implemented 1), but honestly I think 2) would be a better choice. I couldn't figure out the correct incantation for archunit though. |
Hi @andreabergia, had a quick look and i like your commited solution more than changing the architecture test. For my understanding this RegExpProxy is a way to decouple the regexp impl from the rhino core and make it exchangeable. And therefore i think having this init method in the interface is correct. And something like should work... (untested) |
Sounds good. Thanks for checking! |
|
@andreabergia did a quick smoke test with HtmlUnit and it looks good! |
| @@ -188,6 +188,7 @@ public static ScriptableObject initSafeStandardObjects( | |||
|
|
|||
| NativeArrayIterator.init(scope, sealed); | |||
| NativeStringIterator.init(scope, sealed); | |||
| getRegExpProxy(cx).register(scope, sealed); | |||
There was a problem hiding this comment.
I think that by doing this, we're changing the implementation so that it'll bomb if "RegExpImpl" isn't in the classpath, because "getRegExpProxy" will return null in that case. I'm not sure that we have ever taken advantage of that fact, but I think that with the current lazy constructor, a less catastrophic failure will result, and like all things Rhino, we don't really know what people are doing with the general capability to have the reg exp implementation be optional. Is there a different way to do this?
There was a problem hiding this comment.
Changed that in 314de9e
| return ITERATOR_TAG; | ||
| } | ||
|
|
||
| private Scriptable regexp; |
There was a problem hiding this comment.
FWIW, the Rhino creators in the Old Days seemed to like to put variables at the end of the class, but for new stuff I think it's OK (and preferable) if we act like modern-day Java developers and put them at the beginning.
| @@ -2761,7 +2768,7 @@ public Object execIdCall( | |||
| return realThis(thisObj, f).toString(); | |||
|
|
|||
| case Id_exec: | |||
| return realThis(thisObj, f).execSub(cx, scope, args, MATCH); | |||
There was a problem hiding this comment.
I see that we're also changing stuff in IdScriptableObject that's been there a while, so I'm trying to follow this. What is changing here that's requiring us to treat the name of the function object differently?
There was a problem hiding this comment.
The reason is that, in IdScriptableObject, the method ensureType (used by realThis) took an IdFunctionObject. However, that parameter was only used to get the function name. So, I've refactored it and added an overload that only takes the function name.
This allows me to have a method NativeRegExp::js_exec that does not go through the IdFunctionObject at all, and so it can easily be called from NativeRegExpStringIterator to implement point 3 of the spec algorithm RegExpExec.
This includes: - `Symbol.matchAll` - `String.prototype.matchAll` - `RegExp.prototype[Symbol.matchAll]` All test262 cases pass; those that do not, fail for pre-existing reasons unrelated to these changes. Added also some manual tests.
andreabergia
force-pushed
the
string-match-all
branch
from
9d2d4ae to
314de9e
Compare
|
Rebased onto the latest |
|
@gbrail - will be great if you can merge this soon - i plan a new HtmlUnit release during the next days and i like to have this in |
|
OK, works for me, glad you all like it ;-) |
This PR implements:
Symbol.matchAllString.prototype.matchAllRegExp.prototype[Symbol.matchAll]All test262 cases pass; those that do not, fail for pre-existing reasons unrelated to these changes:
flagson regexp #1730thisobject, so we get aNativeStringinstead of the raw string literal and the last assertion failsf.call(true)receivedes aNativeBooleanwhich is an objectPlease squash if accepted!
Closes #933