-
Notifications
You must be signed in to change notification settings - Fork 59
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
Refactor require()
to make it easier to reason about Lua stack usage.
#905
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Disable copy assignment operator as well as copy constructor. Use std::uncaught_exceptions() in destructor to report whether there's an in-flight exception at block exit. Since that was the whole point of the DEBUGIN / DEBUGEND macros, those become obsolete. Ditch them and their existing invocations.
Push throwing Lua errors down into LLRequireResolver::findModule() and findModuleImpl() so their callers don't have to handle the error case. That eliminates finishrequire(). require() itself now only retrieves (and pops) the passed module name and calls LLRequireResolver::resolveRequire() to do the actual work. resolveRequire() is now void. It only instantiates LLRequireResolver and calls its findModule(). findModule() is now also void. It's guaranteed to either push the loaded Lua module or throw a Lua error. In particular, when findPathImpl() cannot find the specified module, findModule() throws an error. That replaces ModuleStatus::NotFound. Since std::filesystem::path::append() aka operator/() detects when its right operand is absolute and, in that case, discards the left operand, we no longer need resolveAndStoreDefaultPaths(): we can just invoke that operation inline. When findModule() pushes _MODULES on the Lua stack, it uses LuaRemover (below) to ensure that _MODULES is removed again no matter how findModules() exits. findModuleImpl() now accepts the candidate pathname as its argument. That eliminates mAbsolutePath. findModuleImpl() now returns only bool: true means the module was found and loaded and pushed on the Lua stack, false means not found and nothing was pushed; no return means an error was reported. Push running a newly found module's source file down into findModuleImpl(). That eliminates the distinction between Cached and FileRead, which obviates ModuleStatus: a bool return means either "previously cached" or "we read it, compiled it, loaded it and ran it." That also eliminates the need to store the module's textual content in mSourceCode. Similarly, once loading the module succeeds, findModuleImpl() caches it in _MODULES right away. That eliminates ResolvedRequire since we need not pass the full pathname of the found module (or its contents) back up through the call chain. Move require() code that runs the new module into private runModule() method, called by findModuleImpl() in the not-cached case. runModule() is the only remaining method that can push either a string error message or the desired module, because of its funny stack manipulations. That means the check for a string error message on the stack top can move down to findModuleImpl(). Add LuaRemover class to ensure that on exit from some particular C++ block, the specified Lua stack entry will definitely be removed. This is different from LuaPopper in that it engages lua_remove() rather than lua_pop(). Also ditch obsolete await_event() Lua entry point.
The code to save the loaded module was using the wrong key.
* converts to an absolute index. The point of LuaRemover is to remove the | ||
* entry at the specified index regardless of subsequent pushes to the stack. | ||
*/ | ||
class LuaRemover |
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.
Ok, I like this addition for clearing the stack. _MODULES is removed much cleaner now.
maxim-productengine
approved these changes
Mar 1, 2024
Add Queue.lua from roblox.com documentation.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Push throwing Lua errors down into
LLRequireResolver::findModule()
andfindModuleImpl()
so their callers don't have to handle the error case. Thateliminates
finishrequire()
.require()
itself now only retrieves (and pops) the passed module name andcalls
LLRequireResolver::resolveRequire()
to do the actual work.resolveRequire()
is nowvoid
. It only instantiatesLLRequireResolver
and callsits
findModule()
method.findModule()
is now alsovoid
. It's guaranteed to either push the loaded Luamodule or throw a Lua error. In particular, when
findModuleImpl()
cannot findthe specified module,
findModule()
throws an error. That replacesModuleStatus::NotFound
.Since
std::filesystem::path::append()
akaoperator/()
detects when its rightoperand is absolute and, in that case, discards the left operand, we no longer
need
resolveAndStoreDefaultPaths()
: we can just invoke that operation inline.When
findModule()
pushes_MODULES
on the Lua stack, it usesLuaRemover
(below)to ensure that
_MODULES
is removed again no matter howfindModules()
exits.findModuleImpl()
now accepts the candidate pathname as its argument. Thateliminates
mAbsolutePath
.findModuleImpl()
now returns onlybool
:true
means the module was found andloaded and pushed on the Lua stack,
false
means not found and nothing waspushed; no return means an error was reported.
Push running a newly found module's source file down into
findModuleImpl()
.That eliminates the distinction between
Cached
andFileRead
, which obviatesModuleStatus
: atrue
return means either "previously cached" or "we read it,compiled it, loaded it and ran it." That also eliminates the need to store the
module's textual content in
mSourceCode
.Similarly, once loading the module succeeds,
findModuleImpl()
caches it in_MODULES
right away. That eliminatesResolvedRequire
since we need not passthe full pathname of the found module (or its contents) back up through the
call chain.
Move
require()
code that runs the new module into privaterunModule()
method,called by
findModuleImpl()
in the not-cached case.runModule()
is the onlyremaining method that can push either a string error message or the desired
module, because of its funny stack manipulations. That means the check for a
string error message on the stack top can move down to
findModuleImpl()
.Add
LuaRemover
class to ensure that on exit from some particular C++ block,the specified Lua stack entry will definitely be removed. This is different
from
LuaPopper
in that it engageslua_remove()
rather thanlua_pop()
.Also ditch obsolete
await_event()
Lua entry point.