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

Increase ops when calling functions in most contexts #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Derpius
Copy link
Member

@Derpius Derpius commented Dec 27, 2023

⚠️ I applied StyLua to the file in this PR, so you'll want to look at the diff from after that commit

Issue

Should resolve upstream issue wiremod#2958

Changes

  • Applies StyLua formatting to the file (this is most of the diff)
  • Updates the ExprCall and ExprMethodCall resolvers to add ops when calling functions
    • Only ExprCall user functions were adding ops

Impact

Prevents calling functions an unlimited number of times (until hitting time quota, if enabled)

Testing

  • Expression calls
    • User functions with @strict add ops equal to the cost of the function
    • User functions without @strict add ops equal to the cost of the function or 15 if not defined, plus 10 if the function is "legacy"
    • Non-legacy function calls add ops equal to the function's cost or 2 if not defined
    • Legacy function calls add ops equal to the function's cost or 2 if not defined, plus 1 for being legacy
  • Expression method calls
    • Same test cases as above, except for method calls (e.g. SomeVar:someFunction())

Note that as I'm unfamiliar with the compiler, I'm unsure on the following two points:

  • What classes a function as "legacy"
    • Function registered with registerFunction and legacy set to either undefined or true in the options
    • Doesn't apply to UDFs, that part of the ops calculation is probably a mistake
  • When a function's cost would be undefined
    • The cost is typed as an always defined integer, but x or 123 checks were done for user functions already, which I've replicated

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.

1 participant