From 230848a6e18015a3c4501f0b368ac73b167edc60 Mon Sep 17 00:00:00 2001
From: ValKmjolnir <lhk101lhk101@qq.com>
Date: Mon, 13 May 2024 00:06:37 +0800
Subject: [PATCH] :memo: improve maintainability

---
 src/nasal_type.cpp | 64 ++++++++++++++++++++++++----------------------
 src/nasal_vm.h     | 59 +++++++++++++++++++++++++++---------------
 test/class.nas     | 14 +++++-----
 test/scalar.nas    | 18 ++++++++++++-
 4 files changed, 96 insertions(+), 59 deletions(-)

diff --git a/src/nasal_type.cpp b/src/nasal_type.cpp
index bef537c..537db2f 100644
--- a/src/nasal_type.cpp
+++ b/src/nasal_type.cpp
@@ -42,6 +42,7 @@ var nas_hash::get_value(const std::string& key) {
     } else if (!elems.count("parents")) {
         return var::none();
     }
+
     auto ret = var::none();
     auto& val = elems.at("parents");
     if (!val.is_vec()) {
@@ -64,12 +65,14 @@ var* nas_hash::get_memory(const std::string& key) {
     } else if (!elems.count("parents")) {
         return nullptr;
     }
+
     var* addr = nullptr;
     var& val = elems.at("parents");
     if (!val.is_vec()) {
         return addr;
     }
     for(auto& i : val.vec().elems) {
+        // recursively search key in `parents`
         if (i.is_hash()) {
             addr = i.hash().get_memory(key);
         }
@@ -102,11 +105,10 @@ void nas_func::clear() {
     keys.clear();
 }
 
-void nas_ghost::set(
-    const std::string& ghost_type_name,
-    destructor destructor_pointer,
-    marker gc_marker_pointer,
-    void* ghost_pointer) {
+void nas_ghost::set(const std::string& ghost_type_name,
+                    destructor destructor_pointer,
+                    marker gc_marker_pointer,
+                    void* ghost_pointer) {
     type_name = ghost_type_name;
     destructor_function = destructor_pointer;
     gc_mark_function = gc_marker_pointer;
@@ -214,14 +216,14 @@ nas_val::nas_val(vm_type val_type) {
 
 nas_val::~nas_val() {
     switch(type) {
-        case vm_type::vm_str:  delete ptr.str;   break;
-        case vm_type::vm_vec:  delete ptr.vec;   break;
-        case vm_type::vm_hash: delete ptr.hash;  break;
-        case vm_type::vm_func: delete ptr.func;  break;
-        case vm_type::vm_upval:delete ptr.upval; break;
-        case vm_type::vm_ghost:delete ptr.obj;   break;
-        case vm_type::vm_co:   delete ptr.co;    break;
-        case vm_type::vm_map:  delete ptr.map;   break;
+        case vm_type::vm_str:   delete ptr.str;   break;
+        case vm_type::vm_vec:   delete ptr.vec;   break;
+        case vm_type::vm_hash:  delete ptr.hash;  break;
+        case vm_type::vm_func:  delete ptr.func;  break;
+        case vm_type::vm_upval: delete ptr.upval; break;
+        case vm_type::vm_ghost: delete ptr.obj;   break;
+        case vm_type::vm_co:    delete ptr.co;    break;
+        case vm_type::vm_map:   delete ptr.map;   break;
         default: break;
     }
     type = vm_type::vm_nil;
@@ -229,14 +231,14 @@ nas_val::~nas_val() {
 
 void nas_val::clear() {
     switch(type) {
-        case vm_type::vm_str:  ptr.str->clear();        break;
-        case vm_type::vm_vec:  ptr.vec->elems.clear();  break;
-        case vm_type::vm_hash: ptr.hash->elems.clear(); break;
-        case vm_type::vm_func: ptr.func->clear();       break;
-        case vm_type::vm_upval:ptr.upval->clear();      break;
-        case vm_type::vm_ghost:ptr.obj->clear();        break;
-        case vm_type::vm_co:   ptr.co->clear();         break;
-        case vm_type::vm_map:  ptr.map->clear();        break;
+        case vm_type::vm_str:   ptr.str->clear();        break;
+        case vm_type::vm_vec:   ptr.vec->elems.clear();  break;
+        case vm_type::vm_hash:  ptr.hash->elems.clear(); break;
+        case vm_type::vm_func:  ptr.func->clear();       break;
+        case vm_type::vm_upval: ptr.upval->clear();      break;
+        case vm_type::vm_ghost: ptr.obj->clear();        break;
+        case vm_type::vm_co:    ptr.co->clear();         break;
+        case vm_type::vm_map:   ptr.map->clear();        break;
         default: break;
     }
 }
@@ -259,16 +261,16 @@ std::string var::to_str() {
 
 std::ostream& operator<<(std::ostream& out, var& ref) {
     switch(ref.type) {
-        case vm_type::vm_none: out << "undefined";   break;
-        case vm_type::vm_nil:  out << "nil";         break;
-        case vm_type::vm_num:  out << ref.val.num;   break;
-        case vm_type::vm_str:  out << ref.str();     break;
-        case vm_type::vm_vec:  out << ref.vec();     break;
-        case vm_type::vm_hash: out << ref.hash();    break;
-        case vm_type::vm_func: out << "func(..) {..}"; break;
-        case vm_type::vm_ghost:out << ref.ghost();   break;
-        case vm_type::vm_co:   out << ref.co();      break;
-        case vm_type::vm_map:  out << ref.map();     break;
+        case vm_type::vm_none:  out << "undefined";   break;
+        case vm_type::vm_nil:   out << "nil";         break;
+        case vm_type::vm_num:   out << ref.val.num;   break;
+        case vm_type::vm_str:   out << ref.str();     break;
+        case vm_type::vm_vec:   out << ref.vec();     break;
+        case vm_type::vm_hash:  out << ref.hash();    break;
+        case vm_type::vm_func:  out << "func(..) {..}"; break;
+        case vm_type::vm_ghost: out << ref.ghost();   break;
+        case vm_type::vm_co:    out << ref.co();      break;
+        case vm_type::vm_map:   out << ref.map();     break;
         default: break;
     }
     return out;
diff --git a/src/nasal_vm.h b/src/nasal_vm.h
index ea175f7..2286b01 100644
--- a/src/nasal_vm.h
+++ b/src/nasal_vm.h
@@ -20,13 +20,15 @@ namespace nasal {
 class vm {
 protected:
 
-    /* registers of vm */
-    context ctx; // running context
+    /* vm context */
+    context ctx;
 
     /* constants */
-    const f64* const_number = nullptr; // constant numbers
-    const std::string* const_string = nullptr; // constant symbols and strings
+    const f64* const_number = nullptr;
+    const std::string* const_string = nullptr;
     std::vector<u64> imm; // immediate number table
+
+    /* nasal native functions */
     std::vector<nasal_builtin_table> native_function;
     
     /* garbage collector */
@@ -77,9 +79,11 @@ protected:
     std::string type_name_string(const var&) const;
     void die(const std::string&);
 
+protected:
     /* vm calculation functions*/
     inline bool cond(var&);
 
+protected:
     /* vm operands */
     inline void o_repl();
     inline void o_intl();
@@ -247,8 +251,9 @@ inline void vm::o_newv() {
     auto& vec = newv.vec().elems;
     vec.resize(imm[ctx.pc]);
     // use top-=imm[pc]-1 here will cause error if imm[pc] is 0
-    ctx.top = ctx.top-imm[ctx.pc]+1;
-    for(u32 i = 0; i<imm[ctx.pc]; ++i) {
+    ctx.top = ctx.top - imm[ctx.pc] + 1;
+
+    for(u64 i = 0; i<imm[ctx.pc]; ++i) {
         vec[i] = ctx.top[i];
     }
     ctx.top[0] = newv;
@@ -266,16 +271,21 @@ inline void vm::o_newf() {
 
     /* this means you create a new function in local scope */
     if (ctx.localr) {
+        // copy upval scope list from upper level function
         func.upval = ctx.funcr.func().upval;
-        // function created in the same local scope shares one closure
-        // so this size & stk setting has no problem
+
+        // function created in the same local scope shares same closure
         var upval = (ctx.upvalr.is_nil())?
             ngc.alloc(vm_type::vm_upval):
             ctx.upvalr;
-        upval.upval().size = ctx.funcr.func().local_size;
-        upval.upval().stack_frame_offset = ctx.localr;
+        // if no upval scope exists, now it's time to create one
+        if (ctx.upvalr.is_nil()) {
+            upval.upval().size = ctx.funcr.func().local_size;
+            upval.upval().stack_frame_offset = ctx.localr;
+            ctx.upvalr = upval;
+        }
+
         func.upval.push_back(upval);
-        ctx.upvalr = upval;
     }
 }
 
@@ -679,6 +689,7 @@ inline void vm::o_callh() {
         die("must call a hash but get "+type_name_string(val));
         return;
     }
+
     const auto& str = const_string[imm[ctx.pc]];
     if (val.is_hash()) {
         ctx.top[0] = val.hash().get_value(str);
@@ -968,25 +979,31 @@ inline void vm::o_mcallv() {
 }
 
 inline void vm::o_mcallh() {
-    var hash = ctx.top[0]; // mcall hash, reserved on stack to avoid gc
+    // mcall hash, reserved on stack to avoid gc, so do not do ctx.top--
+    var hash = ctx.top[0];
     if (!hash.is_hash() && !hash.is_map()) {
-        die("must call a hash/namespace but get "+type_name_string(hash));
+        die("must call a hash/namespace but get " + type_name_string(hash));
         return;
     }
-    const auto& str = const_string[imm[ctx.pc]];
+
+    const auto& key = const_string[imm[ctx.pc]];
+
+    // map is for nasal namespace type, for example `globals`
     if (hash.is_map()) {
-        ctx.memr = hash.map().get_memory(str);
+        ctx.memr = hash.map().get_memory(key);
         if (!ctx.memr) {
-            die("cannot find symbol \"" + str + "\"");
+            die("cannot find symbol \"" + key + "\"");
         }
         return;
     }
+
+    // call hash member
     auto& ref = hash.hash();
-    ctx.memr = ref.get_memory(str);
-    // create a new key
+    ctx.memr = ref.get_memory(key);
+    // create a new key if not exists
     if (!ctx.memr) {
-        ref.elems[str] = nil;
-        ctx.memr = ref.get_memory(str);
+        ref.elems[key] = nil;
+        ctx.memr = ref.get_memory(key);
     }
 }
 
@@ -1024,7 +1041,7 @@ inline void vm::o_ret() {
         auto size = func.func().local_size;
         upval.on_stack = false;
         upval.elems.resize(size);
-        for(u32 i = 0; i<size; ++i) {
+        for(u64 i = 0; i<size; ++i) {
             upval.elems[i] = local[i];
         }
     }
diff --git a/test/class.nas b/test/class.nas
index a28cadc..866ead9 100644
--- a/test/class.nas
+++ b/test/class.nas
@@ -1,12 +1,13 @@
-var student = func(n,a) {
+var student = func(n, a) {
     return {
-        print_info:func println(n,' ',a),
-        set_age:   func(age) a=age,
-        get_age:   func return a,
-        set_name:  func(name) n=name,
-        get_name:  func return n
+        print_info: func println(n,' ',a),
+        set_age:    func(age) a=age,
+        get_age:    func return a,
+        set_name:   func(name) n=name,
+        get_name:   func return n
     };
 }
+
 var s=student('valk',24);
 s.print_info();
 println(s.get_age(),' ',s.get_name());
@@ -18,6 +19,7 @@ s.set_age(20);
 s.set_name('Sidi Liang');
 s.print_info();
 println(s.get_age(),' ',s.get_name());
+
 # flightgear nasal-console cannot use this kind of object initializing
 var m = func() {
     var (_1,_2)=(0,1);
diff --git a/test/scalar.nas b/test/scalar.nas
index 0107198..3773749 100644
--- a/test/scalar.nas
+++ b/test/scalar.nas
@@ -250,4 +250,20 @@ var test_single_id_iterator = 0;
 
 foreach(test_single_id_iterator; [0, 1, 2, 3]) {
     println(test_single_id_iterator);
-}
\ No newline at end of file
+}
+
+# simple closure test, make sure two functions share the same closure
+var closure_tester = func() {
+    var b = 0;
+    return [
+        func { b += 1; },
+        func { return b; }
+    ];
+}();
+
+for(var i = 1; i<=10; i += 1) {
+    closure_tester[0]();
+    if (closure_tester[1]()!=i) {
+        die("test failed: expect " ~ i ~ ", but get " ~ closure_tester[1]());
+    }
+}