Fix test "allocator = <bad C function>" not failing against LuaJIT like it should

4 files changed, 185 insertions, 176 deletions

diff --git a/src/state.cpp b/src/state.cpp
index b558d11..fc7f5ef 100644
--- a/src/state.cpp
+++ b/src/state.cpp
@@ -158,7 +158,7 @@ namespace state {
                         // for some reason, LuaJIT 64 bits does not support creating a state with lua_newstate...
                         return luaL_newstate();
                     } else {
-                        lanes::AllocatorDefinition const _def{ U->resolveAllocator(from, hint) };
+                        lanes::AllocatorDefinition const _def{ U->resolveAndValidateAllocator(from, hint) };
                         return _def.newState();
                     }
                 }
diff --git a/src/universe.cpp b/src/universe.cpp
index dd7bc4b..db00b72 100644
--- a/src/universe.cpp
+++ b/src/universe.cpp
@@ -244,7 +244,7 @@ void Universe::initializeAllocatorFunction(lua_State* const L_)
         provideAllocator = lua_tocfunction(L_, -1);                                                // L_: settings allocator
         if (provideAllocator != nullptr) {
             // make sure the function doesn't have upvalues
-            char const* _upname = lua_getupvalue(L_, -1, 1);                                       // L_: settings allocator upval?
+            char const* _upname{ lua_getupvalue(L_, -1, 1) };                                      // L_: settings allocator upval?
             if (_upname != nullptr) {   // should be "" for C functions with upvalues if any
                 raise_luaL_error(L_, "config.allocator() shouldn't have upvalues");
             }
@@ -266,11 +266,11 @@ void Universe::initializeAllocatorFunction(lua_State* const L_)
     std::ignore = luaG_getfield(L_, kIdxTop, "internal_allocator");                                // L_: settings "libc"|"allocator"
     LUA_ASSERT(L_, lua_isstring(L_, kIdxTop)); // should be the case due to lanes.lua parameter validation
     std::string_view const _allocator{ luaG_tostring(L_, kIdxTop) };
+    // use whatever the provider provides. This performs validation of what provideAllocator is giving
+    // we do this even if _allocator == "libc", to have the validation part
+    internalAllocator = resolveAndValidateAllocator(L_, "internal");
     if (_allocator == "libc") {
         internalAllocator = lanes::AllocatorDefinition{ libc_lua_Alloc, nullptr };
-    } else {
-        // use whatever the provider provides
-        internalAllocator = resolveAllocator(L_, "internal");
     }
     lua_pop(L_, 1);                                                                                // L_: settings
     STACK_CHECK(L_, 1);
@@ -331,7 +331,7 @@ void Universe::initializeOnStateCreate(lua_State* const L_)
 
 // #################################################################################################
 
-lanes::AllocatorDefinition Universe::resolveAllocator(lua_State* const L_, std::string_view const& hint_) const
+lanes::AllocatorDefinition Universe::resolveAndValidateAllocator(lua_State* const L_, std::string_view const& hint_) const
 {
     lanes::AllocatorDefinition _ret{ protectedAllocator };
     if (provideAllocator == nullptr) {
diff --git a/src/universe.hpp b/src/universe.hpp
index 2a3085d..42a3d83 100644
--- a/src/universe.hpp
+++ b/src/universe.hpp
@@ -151,7 +151,7 @@ class Universe final
     static int InitializeFinalizer(lua_State* L_);
     void initializeOnStateCreate(lua_State* L_);
     [[nodiscard]]
-    lanes::AllocatorDefinition resolveAllocator(lua_State* L_, std::string_view const& hint_) const;
+    lanes::AllocatorDefinition resolveAndValidateAllocator(lua_State* L_, std::string_view const& hint_) const;
     static inline void Store(lua_State* L_, Universe* U_);
     [[nodiscard]]
     bool terminateFreeRunningLanes(lua_Duration shutdownTimeout_, CancelOp op_);
diff --git a/unit_tests/init_and_shutdown.cpp b/unit_tests/init_and_shutdown.cpp
index 9652f2d..b8174fd 100644
--- a/unit_tests/init_and_shutdown.cpp
+++ b/unit_tests/init_and_shutdown.cpp
@@ -56,234 +56,243 @@ TEST_CASE("lanes.require 'lanes'")
 }
 
 // #################################################################################################
-// #################################################################################################
-// allocator should be "protected", a C function returning a suitable userdata, or nil
 
-TEST_CASE("lanes.configure")
+// allocator should be "protected", a C function returning a suitable userdata, or nil
+TEST_CASE("lanes.configure.allocator")
 {
     LuaState L{ LuaState::WithBaseLibs{ true }, LuaState::WithFixture{ false } };
 
+    SECTION("allocator = false")
+    {
+        L.requireFailure("require 'lanes'.configure{allocator = false}");
+    }
+
     // ---------------------------------------------------------------------------------------------
 
-    SECTION("allocator", "[allocator]")
+    SECTION("allocator = true")
     {
-        SECTION("allocator = false")
-        {
-            L.requireFailure("require 'lanes'.configure{allocator = false}");
-        }
+        L.requireFailure("require 'lanes'.configure{allocator = true}");
+    }
 
-        // -----------------------------------------------------------------------------------------
+    // ---------------------------------------------------------------------------------------------
 
-        SECTION("allocator = true")
-        {
-            L.requireFailure("require 'lanes'.configure{allocator = true}");
-        }
+    SECTION("allocator = <number>")
+    {
+        L.requireFailure("require 'lanes'.configure{allocator = 33}");
+    }
 
-        // -----------------------------------------------------------------------------------------
+    // ---------------------------------------------------------------------------------------------
 
-        SECTION("allocator = <number>")
-        {
-            L.requireFailure("require 'lanes'.configure{allocator = 33}");
-        }
+    SECTION("allocator = <table>")
+    {
+        L.requireFailure("require 'lanes'.configure{allocator = {}}");
+    }
 
-        // -----------------------------------------------------------------------------------------
+    // ---------------------------------------------------------------------------------------------
 
-        SECTION("allocator = <table>")
-        {
-            L.requireFailure("require 'lanes'.configure{allocator = {}}");
-        }
+    SECTION("allocator = <Lua function>")
+    {
+        L.requireFailure("require 'lanes'.configure{allocator = function() return {}, 12, 'yoy' end}");
+    }
 
-        // -----------------------------------------------------------------------------------------
+    // ---------------------------------------------------------------------------------------------
 
-        SECTION("allocator = <Lua function>")
-        {
-            L.requireFailure("require 'lanes'.configure{allocator = function() return {}, 12, 'yoy' end}");
-        }
+    SECTION("allocator = <bad C function>")
+    {
+        // a C function that doesn't return what we expect should cause an error too
+        // TODO: for some reason, we use os.getenv here because using 'print' as the culprit, the tests deadlock in Release builds
+        L.requireFailure("return type(require 'lanes'.configure{allocator = os.getenv})");
+    }
 
-        // -----------------------------------------------------------------------------------------
+    // ---------------------------------------------------------------------------------------------
 
-        SECTION("allocator = <bad C function>")
-        {
-            // a C function that doesn't return what we expect should cause an error too
-            L.requireFailure("require 'lanes'.configure{allocator = print}");
-        }
+    SECTION("allocator = <string with a typo>")
+    {
+        // oops, a typo
+        L.requireFailure("require 'lanes'.configure{allocator = 'Protected'}");
+    }
 
-        // -----------------------------------------------------------------------------------------
+    // ---------------------------------------------------------------------------------------------
 
-        SECTION("allocator = <string with a typo>")
-        {
-            // oops, a typo
-            L.requireFailure("require 'lanes'.configure{allocator = 'Protected'}");
-        }
+    SECTION("allocator = 'protected'")
+    {
+        // no typo, should work
+        L.requireSuccess("require 'lanes'.configure{allocator = 'protected'}");
+    }
 
-        // -----------------------------------------------------------------------------------------
+    // ---------------------------------------------------------------------------------------------
 
-        SECTION("allocator = 'protected'")
-        {
-            // no typo, should work
-            L.requireSuccess("require 'lanes'.configure{allocator = 'protected'}");
-        }
+    SECTION("allocator = <good custom C allocator>")
+    {
+        // a function that provides what we expect is fine
+        static constexpr lua_CFunction _provideAllocator = +[](lua_State* const L_) {
+            lanes::AllocatorDefinition* const _def{ new (L_) lanes::AllocatorDefinition{} };
+            _def->initFrom(L_);
+            return 1;
+        };
+        lua_pushcfunction(L, _provideAllocator);
+        lua_setglobal(L, "ProvideAllocator");
+        L.requireSuccess("require 'lanes'.configure{allocator = ProvideAllocator}");
+    }
 
-        // -----------------------------------------------------------------------------------------
+    // ---------------------------------------------------------------------------------------------
 
-        SECTION("allocator = <good custom C allocator>")
-        {
-            // a function that provides what we expect is fine
-            static constexpr lua_CFunction _provideAllocator = +[](lua_State* const L_) {
-                lanes::AllocatorDefinition* const _def{ new (L_) lanes::AllocatorDefinition{} };
-                _def->initFrom(L_);
-                return 1;
-            };
-            lua_pushcfunction(L, _provideAllocator);
-            lua_setglobal(L, "ProvideAllocator");
-            L.requireSuccess("require 'lanes'.configure{allocator = ProvideAllocator}");
-        }
+    SECTION("allocator not returning an AllocatorDefinition")
+    {
+        // a function that provides something that is definitely not an AllocatorDefinition, should cause an error
+        static constexpr lua_CFunction _provideAllocator = +[](lua_State* const L_) {
+            lua_newtable(L_);
+            return 1;
+        };
+        lua_pushcfunction(L, _provideAllocator);
+        lua_setglobal(L, "ProvideAllocator");
+        // force value of internal_allocator so that the LuaJIT-default 'libc' is not selected
+        // which would prevent us from calling _provideAllocator
+        L.requireFailure("require 'lanes'.configure{allocator = ProvideAllocator, internal_allocator = 'allocator'}");
+    }
 
-        // -----------------------------------------------------------------------------------------
+    // ---------------------------------------------------------------------------------------------
 
-        SECTION("allocator not returning an AllocatorDefinition")
-        {
-            // a function that provides something that is definitely not an AllocatorDefinition, should cause an error
-            static constexpr lua_CFunction _provideAllocator = +[](lua_State* const L_) {
-                lua_newtable(L_);
-                return 1;
-            };
-            lua_pushcfunction(L, _provideAllocator);
-            lua_setglobal(L, "ProvideAllocator");
-            // force value of internal_allocator so that the LuaJIT-default 'libc' is not selected
-            // which would prevent us from calling _provideAllocator
-            L.requireFailure("require 'lanes'.configure{allocator = ProvideAllocator, internal_allocator = 'allocator'}");
-        }
+    SECTION("allocator returning an AllocatorDefinition with the wrong signature")
+    {
+        // a function that provides something that is too small to contain an AllocatorDefinition, should cause an error
+        static constexpr lua_CFunction _provideAllocator = +[](lua_State* const L_) {
+            // create a full userdata that is too small (it only contains enough to store a version tag, but not the rest
+            auto* const _duck{ static_cast<lanes::AllocatorDefinition::version_t*>(lua_newuserdata(L_, sizeof(lanes::AllocatorDefinition::version_t))) };
+            *_duck = 666777;
+            return 1;
+        };
+        lua_pushcfunction(L, _provideAllocator);
+        lua_setglobal(L, "ProvideAllocator");
+        // force value of internal_allocator so that the LuaJIT-default 'libc' is not selected
+        // which would prevent us from calling _provideAllocator
+        L.requireFailure("require 'lanes'.configure{allocator = ProvideAllocator, internal_allocator = 'allocator'}");
+    }
 
-        // -----------------------------------------------------------------------------------------
+    // -----------------------------------------------------------------------------------------
 
-        SECTION("allocator returning an AllocatorDefinition with the wrong signature")
-        {
-            // a function that provides something that is too small to contain an AllocatorDefinition, should cause an error
-            static constexpr lua_CFunction _provideAllocator = +[](lua_State* const L_) {
-                // create a full userdata that is too small (it only contains enough to store a version tag, but not the rest
-                auto* const _duck{ static_cast<lanes::AllocatorDefinition::version_t*>(lua_newuserdata(L_, sizeof(lanes::AllocatorDefinition::version_t))) };
-                *_duck = 666777;
-                return 1;
-            };
-            lua_pushcfunction(L, _provideAllocator);
-            lua_setglobal(L, "ProvideAllocator");
-            // force value of internal_allocator so that the LuaJIT-default 'libc' is not selected
-            // which would prevent us from calling _provideAllocator
-            L.requireFailure("require 'lanes'.configure{allocator = ProvideAllocator, internal_allocator = 'allocator'}");
-        }
+    SECTION("allocator returning something too small to be a valid AllocatorDefinition")
+    {
+        // a function that provides something that attempts to pass as an AllocatorDefinition, but is not, should cause an error
+        static constexpr lua_CFunction _provideAllocator = +[](lua_State* const L_) {
+            // create a full userdata of the correct size, but of course the contents don't match
+            int* const _duck{ static_cast<int*>(lua_newuserdata(L_, sizeof(lanes::AllocatorDefinition))) };
+            _duck[0] = 666;
+            _duck[1] = 777;
+            return 1;
+        };
+        lua_pushcfunction(L, _provideAllocator);
+        lua_setglobal(L, "ProvideAllocator");
+        // force value of internal_allocator so that the LuaJIT-default 'libc' is not selected
+        // which would prevent us from calling _provideAllocator
+        L.requireFailure("require 'lanes'.configure{allocator = ProvideAllocator, internal_allocator = 'allocator'}");
+    }
+}
 
-        // -----------------------------------------------------------------------------------------
+// #################################################################################################
 
-        SECTION("allocator returning something too small to be a valid AllocatorDefinition")
-        {
-            // a function that provides something that attempts to pass as an AllocatorDefinition, but is not, should cause an error
-            static constexpr lua_CFunction _provideAllocator = +[](lua_State* const L_) {
-                // create a full userdata of the correct size, but of course the contents don't match
-                int* const _duck{ static_cast<int*>(lua_newuserdata(L_, sizeof(lanes::AllocatorDefinition))) };
-                _duck[0] = 666;
-                _duck[1] = 777;
-                return 1;
-            };
-            lua_pushcfunction(L, _provideAllocator);
-            lua_setglobal(L, "ProvideAllocator");
-            // force value of internal_allocator so that the LuaJIT-default 'libc' is not selected
-            // which would prevent us from calling _provideAllocator
-            L.requireFailure("require 'lanes'.configure{allocator = ProvideAllocator, internal_allocator = 'allocator'}");
-        }
-    }
+TEST_CASE("lanes.configure.internal_allocator")
+{
+    LuaState L{ LuaState::WithBaseLibs{ true }, LuaState::WithFixture{ false } };
 
-    // ---------------------------------------------------------------------------------------------
     // internal_allocator should be a string, "libc"/"allocator"
 
-    SECTION("[internal_allocator")
+    SECTION("internal_allocator = false")
     {
-        SECTION("internal_allocator = false")
-        {
-            L.requireFailure("require 'lanes'.configure{internal_allocator = false}");
-        }
+        L.requireFailure("require 'lanes'.configure{internal_allocator = false}");
+    }
 
-        // -----------------------------------------------------------------------------------------
+    // ---------------------------------------------------------------------------------------------
 
-        SECTION("internal_allocator = true")
-        {
-            L.requireFailure("require 'lanes'.configure{internal_allocator = true}");
-        }
+    SECTION("internal_allocator = true")
+    {
+        L.requireFailure("require 'lanes'.configure{internal_allocator = true}");
+    }
 
-        // -----------------------------------------------------------------------------------------
+    // ---------------------------------------------------------------------------------------------
 
-        SECTION("internal_allocator = <table>")
-        {
-            L.requireFailure("require 'lanes'.configure{internal_allocator = {}}");
-        }
+    SECTION("internal_allocator = <table>")
+    {
+        L.requireFailure("require 'lanes'.configure{internal_allocator = {}}");
+    }
 
-        // -----------------------------------------------------------------------------------------
+    // ---------------------------------------------------------------------------------------------
 
-        SECTION("internal_allocator = <Lua function>")
-        {
-            L.requireFailure("require 'lanes'.configure{internal_allocator = function() end}");
-        }
+    SECTION("internal_allocator = <Lua function>")
+    {
+        L.requireFailure("require 'lanes'.configure{internal_allocator = function() end}");
+    }
 
-        // -----------------------------------------------------------------------------------------
+    // ---------------------------------------------------------------------------------------------
 
-        SECTION("internal_allocator = <bad string>")
-        {
-            L.requireFailure("require 'lanes'.configure{internal_allocator = 'gluh'}");
-        }
+    SECTION("internal_allocator = <bad string>")
+    {
+        L.requireFailure("require 'lanes'.configure{internal_allocator = 'gluh'}");
+    }
 
-        // -----------------------------------------------------------------------------------------
+    // ---------------------------------------------------------------------------------------------
 
-        SECTION("internal_allocator = 'libc'")
-        {
-            L.requireSuccess("require 'lanes'.configure{internal_allocator = 'libc'}");
-        }
+    SECTION("internal_allocator = 'libc'")
+    {
+        L.requireSuccess("require 'lanes'.configure{internal_allocator = 'libc'}");
+    }
 
-        // -----------------------------------------------------------------------------------------
+    // ---------------------------------------------------------------------------------------------
 
-        SECTION("internal_allocator = 'allocator'")
-        {
-            L.requireSuccess("require 'lanes'.configure{internal_allocator = 'allocator'}");
-        }
+    SECTION("internal_allocator = 'allocator'")
+    {
+        L.requireSuccess("require 'lanes'.configure{internal_allocator = 'allocator'}");
     }
+}
+
+// #################################################################################################
+
+TEST_CASE("lanes.configure.keepers_gc_threshold")
+{
+    LuaState L{ LuaState::WithBaseLibs{ true }, LuaState::WithFixture{ false } };
 
-    // ---------------------------------------------------------------------------------------------
     // keepers_gc_threshold should be a number in [0, 100]
 
-    SECTION("keepers_gc_threshold")
+    SECTION("keepers_gc_threshold = <table>")
     {
-        SECTION("keepers_gc_threshold = <table>")
-        {
-            L.requireFailure("require 'lanes'.configure{keepers_gc_threshold = {}}");
-        }
+        L.requireFailure("require 'lanes'.configure{keepers_gc_threshold = {}}");
+    }
 
-        // -----------------------------------------------------------------------------------------
+    // ---------------------------------------------------------------------------------------------
 
-        SECTION("keepers_gc_threshold = <string>")
-        {
-            L.requireFailure("require 'lanes'.configure{keepers_gc_threshold = 'gluh'}");
-        }
+    SECTION("keepers_gc_threshold = <string>")
+    {
+        L.requireFailure("require 'lanes'.configure{keepers_gc_threshold = 'gluh'}");
+    }
 
-        // -----------------------------------------------------------------------------------------
+    // ---------------------------------------------------------------------------------------------
 
-        SECTION("keepers_gc_threshold = -1")
-        {
-            L.requireSuccess("require 'lanes'.configure{keepers_gc_threshold = -1}");
-        }
+    SECTION("keepers_gc_threshold = -1")
+    {
+        L.requireSuccess("require 'lanes'.configure{keepers_gc_threshold = -1}");
+    }
 
-        // -----------------------------------------------------------------------------------------
+    // ---------------------------------------------------------------------------------------------
 
-        SECTION("keepers_gc_threshold = 0")
-        {
-            L.requireSuccess("require 'lanes'.configure{keepers_gc_threshold = 0}");
-        }
+    SECTION("keepers_gc_threshold = 0")
+    {
+        L.requireSuccess("require 'lanes'.configure{keepers_gc_threshold = 0}");
+    }
 
-        // -----------------------------------------------------------------------------------------
+    // ---------------------------------------------------------------------------------------------
 
-        SECTION("keepers_gc_threshold = 100")
-        {
-            L.requireSuccess("require 'lanes'.configure{keepers_gc_threshold = 100}");
-        }
+    SECTION("keepers_gc_threshold = 100")
+    {
+        L.requireSuccess("require 'lanes'.configure{keepers_gc_threshold = 100}");
     }
+}
+
+// #################################################################################################
+
+TEST_CASE("lanes.configure.the rest")
+{
+    LuaState L{ LuaState::WithBaseLibs{ true }, LuaState::WithFixture{ false } };
+
 
     // ---------------------------------------------------------------------------------------------
     // nb_user_keepers should be a number in [0, 100]