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
Class support #816
Class support #816
Conversation
if (name.StartsWith("language/statements/class/subclass/builtin-objects/Promise")) | ||
{ | ||
skip = true; | ||
reason = "Promise not implemented"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WAT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small PR and you can enable this... Just need to have the constructor logic use correct methods, like I mentioned 😉
var r = new RegExpInstance(Engine); | ||
r._prototype = PrototypeObject; | ||
|
||
var scanner = new Scanner(regExp, new ParserOptions { AdaptRegexp = true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did this go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was never used, so removed dead code with extreme prejudice.
@@ -14,6 +15,7 @@ public static T ThrowSyntaxError<T>(Engine engine, string message = null) | |||
return default; | |||
} | |||
|
|||
[DoesNotReturn] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saw that in your other PR, perf only? JIT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically it helps the compiler. Can write code like:
string? thisCanBeNull = SomeMethod();
if (thisCanBeNull is null)
ThrowHelper.Throw();
// compiler won't complain about possibly being null as it knows the earlier branch won't return this code
thisCanBeNull.Substring(1,2);
var f = new BindFunctionInstance(Engine) | ||
{ | ||
TargetFunction = thisObj, | ||
BoundThis = thisObj is ArrowFunctionInstance ? Undefined : thisArg, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So no more ArrowFunctionInstance, less code repetition then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they now follow the same logic. We can separate again later if we want to fine-tune, but spec doesn't really talk about it being a separate concept, just configured differently.
if (expression is CallExpression callExpression) | ||
{ | ||
expression = callExpression.Callee; | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The while(true)
is only for this case? not a recursion instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is honestly a hack. The correct way to handle this later is to have parser enable options.Range = true
which should give us start and end index, then if we have the script (string instance), we can substring it. Might even make sense to always provide those out of Esprima without config switch if there's no real performance penalty. So this is something like best-effort, should be replaced with proper solution...
No description provided.