-
Notifications
You must be signed in to change notification settings - Fork 944
Use the scripting engine fake client in VM_Call #2826
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
base: unstable
Are you sure you want to change the base?
Conversation
The scripting engine runtime has a fake client that can be used to call commands inside the scripts, and is re-used throughout the scripting engine life cycle. The `VM_Call` implementation uses a temporary client from a client cache to run a command, but when running `VM_Call` in the context of a script, we don't need to get a temporary client from the cache, and can use the scripting engine client that was already prepared to run the command by the `scriptPrepareForRun()` function in `script.c`. This commit makes this small change in `VM_Call` to use the scripting engine client instead of a temporary client from the client cache. Signed-off-by: Ricardo Dias <[email protected]>
|
@zuiderkwast this is the last PR before opening the PR that moves the Lua engine into its own external module. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #2826 +/- ##
============================================
- Coverage 72.42% 72.41% -0.02%
============================================
Files 128 128
Lines 70366 70369 +3
============================================
- Hits 50961 50956 -5
- Misses 19405 19413 +8
🚀 New features to boost your workflow:
|
Signed-off-by: Ricardo Dias <[email protected]>
Ups, I had to open another 😄 #2835 |
| /* When running a script the context client is already a fake client | ||
| * prepared for running a command on behalf of the real client. | ||
| * This preparation is done by the scriptPrepareForRun() in script.c */ | ||
| c = scriptGetClient(); |
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.
When reusing the client, the clients have different flags. Are we sure that this is correct?
The scripting engine client has:
c->flag.deny_blocking = 1;
c->flag.script = 1;
c->flag.fake = 1;
But the module client has:
c->flag.module = 1;
c->flag.fake = 1;
Assuming that we want to make the flags change, what are the future maintainability implications? If the scripting engine later makes changes to its client, it needs to be compatible with modules reusing it. Does this create an unnecessary coupling?
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.
Good questions.
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 scripting engine client will only be used by VM_Call when the VM_Call is called during the execution of a script.
If for some reason we need to update the scripting engine client flags, we can do that in a backward compatible way because we have access to the API version used by the scripting engine module.
Signed-off-by: Ricardo Dias <[email protected]>
|
the failed tests are not related to this PR |
Signed-off-by: Ricardo Dias <[email protected]>
The scripting engine runtime has a fake client that can be used to call commands inside the scripts, and is re-used throughout the scripting engine life cycle.
The
VM_Callimplementation uses a temporary client from a client cache to run a command, but when runningVM_Callin the context of a script, we don't need to get a temporary client from the cache, and can use the scripting engine client that was already prepared to run the command by thescriptPrepareForRun()function inscript.c.This commit makes this small change in
VM_Callto use the scripting engine client instead of a temporary client from the client cache.