Skip to content

Conversation

metoule
Copy link
Contributor

@metoule metoule commented May 15, 2025

#349 (via commit 0346c9d) removed the reflection code that was used to know the operands of binary and unary operators. Instead, it stored MethodData object as static objects, so that they could directly be used during the resolution process.

That proved to be an ill-advised choice: the resolution process mutates the MethodData class, but since they're stored as static variables, it was not thread-safe: thanks @mats-boisen for the investigation in #364!

This PR instead stores immutable objects as static variables. The MethodData used during resolution is therefore created for each resolution.

Fix #361
Fix #364

@metoule metoule requested a review from davideicardi as a code owner May 15, 2025 20:39
@holdenmai
Copy link
Contributor

@metoule What are your thoughts on having the static fields and updated method signatures be Method base instead of the SimpleMethodSignature? Obviously the actual ones you return would still be the simple class.

This may allow us to transform some things going forward if we make further changes/optimizations around how these operations are looked up.

Copy link
Member

@davideicardi davideicardi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@davideicardi
Copy link
Member

@metoule Thank you!

Just a question to fully understand:
When the MethodData will be created exactly with this new PR?

@metoule
Copy link
Contributor Author

metoule commented May 16, 2025

When the MethodData will be created exactly with this new PR?

In Parser.PrepareOperandArguments, when we call MethodResolution.FindBestMethod(signatures, args): that overload converts the MethodBase to MethodData.

@metoule metoule merged commit 15ef85f into dynamicexpresso:master May 16, 2025
2 checks passed
@metoule metoule deleted the fix_364 branch May 16, 2025 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants