For those who aren't aware, @Helgef made a couple of attempts at implementing module support back in 2019 (pull request #142) and 2020 (pull request #162). Mostly behaviour, syntax and semantics were discussed. This post mostly isn't about that, but about the code.
As I've mentioned before, some time back I started reviewing PR#162 and didn't finish, instead diverting my attention to making major changes to the language and implementation. I had always intended to finish reviewing the code, figuring that I might gain some insights about implementing module support or improving the implementation in other ways.
So today I reviewed a diff of PR#162, which went a lot quicker than my previous attempt (reviewing each commit individually). I provide my thoughts below, for any interested reader. If I say "sm", I'm referring to the branch which contains the changes of PR#162.
Superseded
My initial review of sm inspired some changes to the syntax and implementation. sm was written mid-2020, and I'd guess that Helgef would do some things differently if he reproduced the module syntax today.
LineIsPrecededByParentLine() checks if the previous line is a parent line (like if/else/while/etc.), literally just mLastLine && ACT_IS_LINE_PARENT(mLastLine->mActionType). I would tend to avoid creating a helper function for something so simple, especially if defining and declaring the function signature is just as much code as the check itself. Because of changes in the way that blocks and lines are parsed, today I'm pretty sure this function could be replaced with a simple check of mPendingParentLine.
Line::mModule, g_CurrentModule
AutoHotkey already relies on a number of global variables during script execution. I would like to move away from this practice, not create more. For an example of a better design, consider Lua, which is written in C. Any functions that depend on or alter the script state take a parameter of type lua_State*. (But that's outside the scope of this PR or topic.)
mModule is copied to g_CurrentModule each and every time any line of code is executed, but I'd guess that g_CurrentModule isn't needed most of the time; only when a dynamic reference is being evaluated. This seems quite inefficient. Any code that needs the current module should be able to determine the current line, and from that, get the module.
As far as I can tell, a function cannot begin in one module and end in another module. Therefore, any line in a function necessarily belongs to the same module as the function. If mModule was a property of the function rather than each line, it would already be available through g->CurrentFunc->mModule. However, that won't work for code that isn't inside a function. One possible way to solve that would be to enclose module code in an anonymous function.
If we're introducing new complexity to the process of resolving names, it may be helpful to take into account future needs. For instance, lexically scoped variables are visible only within a given block, more specific than a function. Where before we had only global and function scope, in future we might have scope associated with a function block, a module block, or any other block.
So instead of associating a module with each line, we can associate each block-begin with a scope. Determining the scope within which to resolve a variable then becomes a task of locating the block-begin above the current line. This is a fairly trivial task, because each Line has a pointer to its mParentLine. Currently the block-begin belonging to a function has a pointer to the function; this would be replaced with something more general identifying the scope or function.
AddVar, FindVar, etc.
In order to support modules, sm adds a parameter to these functions for which module to add or find the variable in. However, local variables are still dependent on g->CurrentFunc, where g is global but dependent on the pseudo-thread, and CurrentFunc is updated when entering/leaving a function. AddVar handles built-in variables in a way completely independent of the module system, but ideally they would come from the std module.
sm also adds an optional parameter for AddVar to add an existing Var, in which case some parts of the function aren't relevant. I think that this has been completely superseded by changes since sm; i.e. VarList::Insert() would be used instead.
Future needs such as lexically scoped variables would require further changes to AddVar/FindVar. As with blocks (above), I feel that these functions should be revised to support scopes/namespaces in a more general way before (or while) implementing either modules or lexically scoped variables.
// Warning: This macro declares a variable outside the for block
#define FOR_EACH_MODULE(mod) int Macro_Index = 0; for ( ScriptModule *mod; mod = g_script.mModuleSimpleList.GetItem(Macro_Index); ++Macro_Index)
I think the commonplace C++ solution would be to define an iterator and use a proper for loop. For instance, the object defines a begin() iterator which points to the start of the list, an end() iterator which points to the end + 1, and a ++ operator which moves the iterator to the next item. Then for (auto *mod : g_script.mModuleSimpleList) can be used to iterate it.
I don't use this pattern, because in the common case of iterating from 0 to count-1, for (int i = 0; i < count; ++i) tends to produce slightly smaller compiled code than for (int i = 0; i != count; ++i), which is how the iterator pattern ends up looking to the compiler.
In general, it's not possible to declare two different types of variable in the for-loop's initializer, hence Macro_Index being declared outside the loop. However, the macro could have defined an extra block so that it would be self-contained, and wouldn't need to provide a warning.
For this case, there's actually a more elegant way to confine both variables to the for block:
for (int Macro_Index = 0; ScriptModule *mod = g_script.mModuleSimpleList.GetItem(Macro_Index); ++Macro_Index)
Misc
Unless I'm writing code for both v1 and v2 (since v1 is still built with Visual C++ 2010), I tend to prefer using default member initializers. In 2010, it is necessary to list an initializer for each member in the constructor. If you have multiple constructors, it may be necessary to list the same members in each constructor. This repetition is inelegant and error-prone. For example (this is a modified excerpt from sm):
TCHAR result_buf[MAX_NUMBER_LENGTH]; is used with ResultToken for short string return values. In this case it doesn't end up getting used, but if it did, it would be insufficient. The result buffer needs to be MAX_NUMBER_LENGTH + 1 (MAX_NUMBER_SIZE) characters.
// Note: It seems like the loop isn't needed, i.e., it is inconsequentail to check mOuterFunc->mOuterFunc.
This part of the code would need to be rewritten due to changes since sm, but anyway, the loop was in fact needed to cover cases like the following.
global x
outerouter() {
local x
outer() {
inner() {
(x) ; should be outerouter's x, not global x
}
}
}
Cleanup
Modules are deleted in Script::TerminateApp(), by iterating over the module list and deleting each module, then finally deleting the list itself. However, ~SimpleList() already includes code for freeing each item (which is skipped at runtime in this case), and ~SimpleList<ScriptModule*>() is called from ~Script() on exit. Modules should be cleaned up that way.
Freeing individual memory allocations on exit means larger code and more CPU cycles used, without any direct benefit. This is because 1) the C runtime already frees the entire heap when it unloads, and 2) the OS reclaims all memory allocated by the process when it exits. When we're in TerminateApp(), we know the process is exiting, so there's no need to do this cleanup.
However, maybe the destructors do more than free memory, or maybe they will in future. In future, maybe the ~Script() destructor will be called at some time other than process exit, in which case having it clean up properly becomes more important and useful. (TerminateApp originally include some other cleanup code, which I have recently moved in order to better support the use of AutoHotkey as a dll.)
Use of Objects
Originally I used an Object to map unresolved class names to objects during load. The Object stored exactly what was needed in a way that could be easily enumerated, and very little new code was needed.
sm replaces this with an Array, pushing a sequence of values reinterpreted as __int64, and popping them off in reverse order. I would never have even thought of doing it this way, and don't think it would have even been easier than the alternatives. Putting the values into a statically-typed UnresolvedClass struct would be much cleaner/more maintainable.
sm adds a template class named SimpleList that at first glance might seem to be suitable for avoiding boilerplate realloc code, but it has some unnecessary features that complicate matters and would probably increase code size. I've generally avoided std::vector and other STL classes for similar reasons.
A quick fix is to define struct UnresolvedClass and use FlatVector<UnresolvedClass>. FlatVector defines the basic operations we need, although it was originally designed for the purpose of conserving memory usage in objects (the length and size are stored with the allocated data, so a string buffer or list of zero capacity takes up less space).
This implementation reduces compiled code size by about 400 bytes, and makes the code a bit more maintainable.
diff --git a/source/script.cpp b/source/script.cpp
index 63853410..b5303d9b 100644
--- a/source/script.cpp
+++ b/source/script.cpp
@@ -510,7 +510,7 @@ Script::Script()
, mFirstTimer(NULL), mLastTimer(NULL), mTimerEnabledCount(0), mTimerCount(0)
, mFirstMenu(NULL), mLastMenu(NULL), mMenuCount(0)
, mOpenBlock(NULL), mNextLineIsFunctionBody(false), mNoUpdateLabels(false)
- , mClassObjectCount(0), mUnresolvedClasses(NULL), mClassProperty(NULL), mClassPropertyDef(NULL)
+ , mClassObjectCount(0), mUnresolvedClasses(), mClassProperty(NULL), mClassPropertyDef(NULL)
, mCurrFileIndex(0), mCombinedLineNumber(0)
, mFileSpec(_T("")), mFileDir(_T("")), mFileName(_T("")), mOurEXE(_T("")), mOurEXEDir(_T("")), mMainWindowTitle(_T(""))
, mScriptName(NULL)
@@ -6561,7 +6561,7 @@ ResultType Script::DefineClass(LPTSTR aBuf)
Object *outer_class, *base_class = Object::sClass, *base_prototype = Object::sPrototype;
Var *class_var;
ExprTokenType token;
- bool save_class_object = false; // If the base class isn't found, save the class obejct in the unresolved list.
+ UnresolvedClass *unresolved = nullptr;
for (cp = aBuf; *cp && !IS_SPACE_OR_TAB(*cp); ++cp);
if (*cp)
{
@@ -6577,24 +6577,27 @@ ResultType Script::DefineClass(LPTSTR aBuf)
{
// This class or module hasn't been defined yet, but it might be. Store it in the "unresolved" list.
// The list is checked after all modules and classes are loaded, if any can't be resolved the program will terminate.
+
+ if (mUnresolvedClasses.Capacity() == mUnresolvedClasses.Length()
+ && !mUnresolvedClasses.SetCapacity(mUnresolvedClasses.Length() + 100))
+ return ScriptError(ERR_OUTOFMEM);
+ unresolved = mUnresolvedClasses.Value() + mUnresolvedClasses.Length()++; // Must call Length() again because data might have been reallocated.
// Copy the name for later use:
LPTSTR base_class_copy = _tcsdup(base_class_name); // this will be freed when resolving this base class.
- if (!base_class_copy || (!mUnresolvedClasses && !(mUnresolvedClasses = Array::Create()))) // Create the list if it doesn't exist.
+ if (!base_class_copy) // Create the list if it doesn't exist.
return ScriptError(ERR_OUTOFMEM);
// See ResolveScopedClasses for more details:
- APPEND_UNRESOLVED((__int64)(INT_PTR)base_class_copy);
- APPEND_UNRESOLVED((__int64)(INT_PTR)g_CurrentModule);
- APPEND_UNRESOLVED(GetFileLineInfo());
+ unresolved->base_class_name = base_class_copy;
+ unresolved->base_class_module = g_CurrentModule;
+ unresolved->file_line = GetFileLineInfo();
// The base class will be created when its definition is encountered,
// the class which is now being defined will be assigned its base class after the script has loaded
// See Script::ResolveClasses()
base_prototype = NULL; // Setting these to NULL isn't really necessary but done for clarity and maintainabillity.
base_class = NULL;
- save_class_object = true; // Let the below know that we need to add this new class object to the unresolved list.
-
}
else
base_prototype = (Object *)base_class->GetOwnPropObj(_T("Prototype"));
@@ -6681,10 +6684,10 @@ ResultType Script::DefineClass(LPTSTR aBuf)
// reference to the class.
class_object->Release();
- if (save_class_object)
+ if (unresolved)
// the base class of this class doesn't exist yet, so add this class
// to the unresolved list so its base can be found later.
- APPEND_UNRESOLVED((__int64)class_object);
+ unresolved->class_object = class_object;
return OK;
}
@@ -7005,8 +7008,8 @@ Object *Object::GetUnresolvedClass(LPTSTR &aName)
ResultType Script::ResolveClasses()
{
// This function is only meant to be called at load time..
- if (!mUnresolvedClasses)
- return OK;
LPTSTR base_class_name; // The string which followed the extends keyword in a class definition for which the base couldn't be resolved earlier. Always contains at least one dot (.).
Object* base_object; // The base which will be resolved by the above string
Object* class_object; // The class object which was defined by the class name preceeding the extends keyword
@@ -7019,7 +7022,8 @@ ResultType Script::ResolveClasses()
// Free() is called below for maintainability.
ResultToken result;
result.InitResult(result_buf);
- while (mUnresolvedClasses->Length())
+ auto unresolved = mUnresolvedClasses.Value();
+ for (int i = 0; i < mUnresolvedClasses.Length(); ++i, ++unresolved)
{
// Pop, LineInfo, A, B and C.D in:
/*
@@ -7033,21 +7037,11 @@ LineInfo: class B extends C.D {
}
*/
- // Pop the class object (B)
- POP_UNRESOLVED(result);
- class_object = (Object*)result.object;
-
- // Pop the line info (LineInfo) for error reporting.
- POP_UNRESOLVED(result);
- line_file_info = result.value_int64; // this line info points to the line: "class B extends C.D"
-
- // Pop the module in which the class was defined (A)
- POP_UNRESOLVED(result);
- mod = (ScriptModule*)result.value_int64;
+ class_object = unresolved->class_object;
+ line_file_info = unresolved->file_line;
+ mod = unresolved->base_class_module;
+ base_class_name = unresolved->base_class_name;
- // Pop the "extends parameter" (C.D)
- POP_UNRESOLVED(result);
- base_class_name = (LPTSTR)result.value_int64; // Eg, the string "C.D".
// Find the base object:
base_object = mod->FindClassFromDotDelimitedString(base_class_name);
if (!base_object)
@@ -7073,8 +7067,7 @@ LineInfo: class B extends C.D {
} // end while loop
// All bases were resolved!
- mUnresolvedClasses->Release();
- mUnresolvedClasses = NULL;
+ mUnresolvedClasses.Free();
return OK;
}
diff --git a/source/script.h b/source/script.h
index 23f5ce4a..d7a3278f 100644
--- a/source/script.h
+++ b/source/script.h
@@ -2908,6 +2908,14 @@ struct DerefList
DerefType *Last() { return items + count - 1; }
};
+struct UnresolvedClass
+{
+ LPTSTR base_class_name;
+ ScriptModule *base_class_module;
+ __int64 file_line;
+ Object *class_object;
+};
+
class Script
{
private:
@@ -2945,9 +2953,7 @@ private:
Object *mClassObject[MAX_NESTED_CLASSES]; // Class definition currently being parsed.
TCHAR mClassName[MAX_CLASS_NAME_LENGTH + 1]; // Only used during load-time.
- Array *mUnresolvedClasses;
-#define APPEND_UNRESOLVED(item) ARRAY_APPEND(mUnresolvedClasses, item, FAIL) // See Script::DefineClass()
-#define POP_UNRESOLVED(result) ARRAY_POP_(mUnresolvedClasses, result) // See Script::ResolveClasses()
+ FlatVector<UnresolvedClass> mUnresolvedClasses;
Array *mClasses = nullptr;
Property *mClassProperty;
Use of Script Functions
Script functions have a considerable amount of overhead, in terms of both CPU time and code size (perhaps in part due to inlining by the compiler). Using script functions from within C++ is ugly. sm uses StrSplit and InStr internally. I can only imagine this was done because Helgef is familiar with these functions and not familiar with C/C++. Still, I had trouble understanding why anyone would choose to implement it this way.
In one case, StrSplit is used to parse a simple comma-delimited list of names for UseVar/UseFunc. There are similar examples to draw from, like variable declarations, function parameter lists, etc. that are parsed using relatively efficient C++ code. It is quite trivial to do this without resorting to calling script functions or dynamically allocating an array.
This reduced code size only a little, but is somewhat cleaner.
diff --git a/source/ScriptModules/ScriptModules.h b/source/ScriptModules/ScriptModules.h
index abdd868c..c855acf2 100644
--- a/source/ScriptModules/ScriptModules.h
+++ b/source/ScriptModules/ScriptModules.h
@@ -81,8 +81,6 @@ public:
ResultType AddObject(UseParams* aObjs);
ResultType FindAndAddFunc(LPTSTR aFuncName, ScriptModule* aModule, bool aAllowConflict = false);
ResultType AddAllFuncs(ScriptModule* aModule);
- ResultType AddFuncFromList(Array* aFuncList, ScriptModule* aModule);
- ResultType AddVarFromList(Array *aVarList, ScriptModule* aModule);
ResultType FindAndAddVar(LPTSTR aVarName, int aNameLength, ScriptModule* aModule);
ResultType AddAllVars(ScriptModule* aModule);
ResultType AddVar(Var* aVar);
diff --git a/source/ScriptModules/ScriptModulesAddNames.h b/source/ScriptModules/ScriptModulesAddNames.h
index cb34df99..eee6459b 100644
--- a/source/ScriptModules/ScriptModulesAddNames.h
+++ b/source/ScriptModules/ScriptModulesAddNames.h
@@ -64,33 +64,16 @@ ResultType ScriptModule::AddObject(UseParams* aObjs)
if (!mod)
return FAIL;
- // Call BIF_StrSplit:
- // Array: = StrSplit(String, Delimiters, OmitChars, MaxParts : = -1)
- CALL_BIF(StrSplit, strsplit_result,
- aObjs->param1, // String
- _T(","), // Delimiters: comma
- _T(" \t")) // OmitChars: spaces and tabs
- if (CALL_TO_BIF_FAILED(strsplit_result))
- return FAIL; // This is probably out of memory, strsplit already displayed the error.
- Array* arr = (Array*)TokenToObject(strsplit_result); // Array
+ if (aObjs->type_symbol == SYM_FUNC && mod == g_StandardModule) // The standard module is not supported.
+ return AddObjectError(ERR_SMODULES_NOT_SUPPORTED, SMODULES_STANDARD_MODULE_NAME);
- switch (aObjs->type_symbol)
+ for (LPTSTR cp_end, cp = aObjs->param1; *cp; cp = cp_end)
{
- case SYM_FUNC: return AddFuncFromList(arr, mod);
- case SYM_VAR: return AddVarFromList(arr, mod);
+ cp = omit_leading_whitespace(cp);
+ for (cp_end = cp; *cp_end && *cp_end != ','; ++cp_end);
+ auto len = rtrim(cp, cp_end - cp);
- }
- // This shouldn't be reached:
- return FAIL;
-}
-ResultType ScriptModule::AddFuncFromList(Array* aFuncList, ScriptModule* aModule)
-{
- // Adds all functions in aFuncList if found in aModule, to this module
- if (aModule == g_StandardModule) // The standard module is not supported.
- return AddObjectError(ERR_SMODULES_NOT_SUPPORTED, SMODULES_STANDARD_MODULE_NAME);
- ARRAY_FOR_EACH(aFuncList, i, result)
- {
- if (!FindAndAddFunc(result.marker, aModule))
+ if ( !(aObjs->type_symbol == SYM_FUNC ? FindAndAddFunc(cp, mod) : FindAndAddVar(cp, (int)len, mod)) )
return FAIL; // Message already shown.
}
return OK;
@@ -173,16 +156,6 @@ ResultType ScriptModule::AddAllFuncs(ScriptModule* aModule)
return OK;
}
-ResultType ScriptModule::AddVarFromList(Array* aVarList, ScriptModule* aModule)
-{
- ARRAY_FOR_EACH(aVarList, i, result)
- {
- if (!FindAndAddVar(result.marker, (int)result.marker_length, aModule))
- return FAIL;
- }
- return OK;
-
-}
ResultType ScriptModule::FindAndAddVar(LPTSTR aVarName, int aNameLength, ScriptModule* aModule)
{
if (!_tcsicmp(aVarName, SMODULES_IMPORT_NAME_ALL_MARKER))
In another case, StrSplit and InStr are used together to parse a full class name (module.outer.inner). It calls StrSplit(aString, ".") first to get ["module", "outer", "inner"], iterates over this, and on each iteration it calls InStr(aString, ".",,, A_Index) to find the relevant component within aString, which it would already have if it just parsed the string directly.
There was already code for parsing this dot-delimited list, and in fact, that code wasn't removed but isn't fully utilized by sm, because it is only called when there are no dots. It would have been much better to update that code to resolve modules first, then a global variable, then nested classes.
This reduced code size by nearly 1KB.
Diff vs 210ae36b
(I also removed FindClassFromDotDelimitedString, but omitted it from the diff. The linker would discard it anyway.)
Although my memory about the details of the implementation is hazy, to say the least, I can say that I had little or no concerns about code size or code efficiency. My intention was a first draft of working and usable code. If there had ever been an agreement about design, many things would change ofc. BIFs and objects and most other choices I made about the implementation were efficient for me getting the job done.
This implementation reduces compiled code size by about 400 bytes, and makes the code a bit more maintainable.
I recall thinking, at the time, that either I implement this or no one else does (in the foreseeable future). That is, I'd rather have a useful version of modules today, than a useful version of modules which takes a few kb of space less x years in the future. Maintainability, I agree, it is a good thing to prioritise, but I think it conflicts with some of you ideas about code size. Just let the compiler handle such optimisations, by telling it. Why is the compiler setup for speed optimisation and not size if you want size optimisations? (you can have both by proper use of #pragmas). It is a futile approach anyways. Whatever produces smaller code today, might not in the future. Imo, the reason ahk exe isn't 1gb isn't because of concerns about byte-level optimisation, but the choice to not include 300 third-party dependencies.
It wasn't about optimizing code size, but about a more proper implementation, which happens to produce smaller code. In this case, I measured the code size to validate my assumption that it would produce smaller code. If you know that one way to implement a particular pattern is more efficient than another with no particular cost to development time or other things, then you can write better code in future.