-
-
Notifications
You must be signed in to change notification settings - Fork 559
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
Fixes delegates created from expressions. #585
Changes from 1 commit
b0bf973
857b6d6
73bd4c0
bf08e01
564282b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
using System; | ||
using System.Globalization; | ||
using System.Reflection; | ||
using System.Linq; | ||
using Jint.Native; | ||
using Jint.Native.Function; | ||
|
||
|
@@ -37,6 +38,12 @@ public DelegateWrapper(Engine engine, Delegate d) | |
public override JsValue Call(JsValue thisObject, JsValue[] jsArguments) | ||
{ | ||
var parameterInfos = _d.Method.GetParameters(); | ||
|
||
if (parameterInfos.Length > 0 && parameterInfos[0].ParameterType.FullName == "System.Runtime.CompilerServices.Closure") | ||
{ | ||
parameterInfos = parameterInfos.Skip(1).ToArray(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Skip is a bit nasty as it uses LINQ and doesn't perform that well, especially on full framework, but this is fortunately quite isolated case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I updated the code to more performantly use Array.Copy, it looks like it is about 0.1 ticks faster on average, but is a fair amount more complicated. Alternatively I could use ArraySegment<> (or Span<> in C# 7), but that would change the signature downstream and make the non-NETFRAMEWORK case less performant. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it was bit of nitpicking, in Jint codebase there should be as little Thanks for considering this too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed lol. I'm used to code bases where an extra object allocation is the least of it's sins 😄 |
||
} | ||
|
||
int delegateArgumentsCount = parameterInfos.Length; | ||
int delegateNonParamsArgumentsCount = _delegateContainsParamsArgument ? delegateArgumentsCount - 1 : delegateArgumentsCount; | ||
|
||
|
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.
Is typeof possible here?
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.
That was my first inclination, but System.Runtime.CompilerServices.Closure doesn't exist in dotnet framework - I tried adding and using pre-processor directives to only check if dotnet standard, but it didn't work with testing.
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.
Have your tried wrapping the code with
Bot test case and this addition
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.
I had only done it on the addition before - I just applied it to both and now it works, but it is basically ignoring the test case, which is probably not desirable. Does it get tested with both flags?
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.
Could you please add this line to appveyor.yml which is in the root folder:
That should include the test in CI builds.
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.
Done 👍