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

Update RegExpPrototype.cs #272

Closed
wants to merge 1 commit into from
Closed

Update RegExpPrototype.cs #272

wants to merge 1 commit into from

Conversation

RusKnyaz
Copy link
Contributor

RegExp result must be mutable array.

RegExp result should be mutable array.
@sebastienros
Copy link
Owner

Looking at the specification, this line should not be removed, but replaced by this:

array.DefineOwnProperty("length", new PropertyDescriptor(value: lengthValue + 1), true);

This will have the same effect on immutability, fix the correct value, and be compliant. As all the tests passed it would make sense to add a new
one to ensure the behavior is not regressed later. At least on the fact that the length has the correct value and the array is mutable.

Thanks

@sebastienros
Copy link
Owner

@sebastienros
Copy link
Owner

I made this PR based on the specification.
#273

Do you have some code that triggered the issue you are seeing so that I can add it to the tests?

@RusKnyaz
Copy link
Contributor Author

RusKnyaz commented Mar 1, 2016

Hi! Thanks for you answer and sorry for the delayed response.

I haven't checked your decision yet and haven't dealt with your tests (them much, and at me they aren't stable). I plan to make it soon.

I develop headless .net web browser and met two issues with jquery+regexp. So the first issue is immutable regexp result. The sample code below:

var match = /quick\s(brown).+?(jumps)/ig.exec('The Quick Brown Fox Jumps Over The Lazy Dog');
match.shift();

match.shift() or match.slice() - often used in jquery.

FYI, the second issue related with .net RegEx class and line endings handling (\r\n - \n) The two snippets below should produce the same results:

var rheaders = /^(.?):[ \t]([^\r\n]*)$/mg;
var headersString = 'X-AspNetMvc-Version: 4.0\nX-Powered-By: ASP.NET\n\n';
while ( match = rheaders.exec( headersString ) ) {
   console.log(match[1].toLowerCase());
   console.log(match[ 2 ]);
}

var rheaders = /^(.?):[ \t]([^\r\n]*)$/mg;
var headersString = 'X-AspNetMvc-Version: 4.0\r\nX-Powered-By: ASP.NET\r\n\r\n';
while ( match = rheaders.exec( headersString ) ) {
   console.log(match[1].toLowerCase());
  console.log(match[ 2 ]);
} Currenlty i solved the issue by modifying the regex pattern replacing $ symbol with specific sequence to match line endings.

Вторник, 1 февраля 2016, 1:23 +06:00 от Sébastien Ros notifications@github.com:

I made this PR based on the specification.
#273
Do you have some code that triggered the issue you are seeing so that I can add it to the tests?

Reply to this email directly or view it on GitHub .

С уважением,
Константин Петухов
petuhov_k@mail.ru

@sebastienros
Copy link
Owner

Be aware that there are discrepancies between .NET and JS in terms of regular expressions evaluation. There are some unit tests from ECMA that are ignored in Jint's code base because of that. If you look into the skipped tests you will find which ones.

@sebastienros
Copy link
Owner

I implemented everything with this PR:
#273

@RusKnyaz
Copy link
Contributor Author

RusKnyaz commented Mar 7, 2016

About multiline.

What if source pattern contains escaped '' before '$'? Something like ".*$".

Suggest to try this:

var replaceRegex = new Regex(@"*$", RegexOptions.Compiled) ;
 
var modifiedFilter = replaceRegex.Replace(sourceFilter, match =>
    match.Value.Length % 2 == 1 ? match.Value.Substring(0, match.Length - 1) + @"\r?$" : match.Value);

Воскресенье, 6 марта 2016, 11:59 -08:00 от Sébastien Ros notifications@github.com:

I implemented everything with this PR:
#273

Reply to this email directly or view it on GitHub .

С уважением,
Константин Петухов
petuhov_k@mail.ru

@sebastienros
Copy link
Owner

Good catch, I will fix it.

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.

None yet

2 participants