Modernize lua read (part 2 & 3): C++ templating assurance #7410

nerzhul · 2018-06-04T21:16:57Z

Implement the boolean reader
Also remove unused & unimplemented script_error_handler
move readers to a new LuaHelper class permitting to share code between Lua interfaces

Implement the boolean reader Also remove unused & unimplemented script_error_handler

nerzhul · 2018-06-04T21:18:13Z

src/script/common/helper.cpp

+template<>
+bool LuaHelper::readParam(lua_State *L, int index)
+{
+	return lua_toboolean(L, index) != 0;


a question: should i add a throw LuaError with not a bool here to use strong bool or not ? we are lazy on checks

Implement the std::string reader Also be strict with boolean checks, C++ doesn't define false as the boolean default value, it's undef Fix some wrong type checks in some code parts, detected when enabled string and boolean strict type checking

nerzhul · 2018-06-05T06:26:50Z

Note, the parts are not to be squashed. I tested it with mtg without having problems, but the strict checking on bool can either fix some random bugs (undefined default bool in some calls) or permit mods to fix some implementation bugs

nerzhul · 2018-06-05T06:27:48Z

Note to @SmallJoker : LuaError is catched by serverstep or client step and shown with a backtrace properly :)

SmallJoker · 2018-06-05T08:17:42Z

src/script/common/helper.cpp

+		return "";
+
+	if (!lua_isstring(L, index))
+		throw LuaError(luahelper_type_error(index, "string"));


luaL_checkstring does the same in one line.

SmallJoker · 2018-06-05T08:19:07Z

src/script/common/helper.cpp

+template <> std::string LuaHelper::readParam(lua_State *L, int index)
+{
+	if (lua_isnil(L, index))
+		return "";


In some places it's best to throw an error since a proper string value is expected.

i agree with you, first i didn't added this but we had many stack traces with regular MTG :) we can remove it later after fixing our upstream calls or i can remove it and we will fix it on each bug reported by end users

That's indeed bad. I would however prefer to fix those issues instead of silently accepting it.

i remove it, and we are free to fix the engine crashes after :)

SmallJoker · 2018-06-05T08:20:18Z

src/script/cpp_api/s_security.cpp

@@ -649,7 +649,7 @@ int ScriptApiSecurity::sl_g_loadfile(lua_State *L)
 	lua_pop(L, 1);

 	if (script->getType() == ScriptingType::Client) {
-		std:: string display_path = lua_tostring(L, 1);
+		std:: string display_path = readParam<std::string>(L, 1);


Space after ::

SmallJoker · 2018-06-05T08:21:16Z

src/script/lua_api/l_object.cpp

@@ -243,7 +243,8 @@ int ObjectRef::l_set_hp(lua_State *L)
 		lua_pushvalue(L, 3);

 		lua_getfield(L, -1, "type");
-		if (lua_isstring(L, -1) && !reason.setTypeFromString(lua_tostring(L, -1))) {
+		if (lua_isstring(L, -1) &&
+			!reason.setTypeFromString(readParam<std::string>(L, -1))) {


Missing indent.

SmallJoker · 2018-06-05T08:22:59Z

src/script/common/helper.cpp

+		result.append(str);
+	}
+
+	return str;


Shouldn't this be return result;?

paramat · 2018-06-28T16:52:19Z

What's the advantage of this, is lua parameter reading faster?

nerzhul · 2018-06-28T17:21:20Z

@paramat it's exactly same performance (in release mode, negligible in debug mode). The advantage if we implement the template on all types is that permit to access to lua reader/writer in a uniform, standardized and securized way. One high level function per type to perform the call with all security/sanitize it needs. it also permits to implement easily a complex reader in the same API (imagine writeParam<Json::Value>(L, json_val), for example)

It's basically a code quality implementation, and it can be (at the end) easily tested with unittests if we need it

paramat · 2018-06-28T18:04:20Z

Ok thanks, no objection from me. Unfortunately i'm not qualified to officially +1 it.

nerzhul · 2018-06-30T10:02:34Z

@SmallJoker if you are okay, i can merge this. It seems other qualified devs are not concerned about this

nerzhul · 2018-06-30T11:03:39Z

Approved by @SmallJoker on IRC

This reverts commit ceead47.

This reverts commit 3e4c584.

This reverts commit 2829aa1.

This reverts commit e5c01a1.

This reverts commit baf9288.

Also added luaL_checkboolean to have a properly implemented boolean check

sfan5 · 2018-06-30T12:12:47Z

src/script/common/helper.cpp

+ */
+template <> bool LuaHelper::readParam(lua_State *L, int index)
+{
+	return lua_toboolean(L, index) != 0;


should have a comment why luaL_checkboolean is not used here

this function doesn't exists in Lua & has been dropped from the header, as it seems useless currently (i added this in a previous version of this PR)

sfan5 · 2018-06-30T12:12:56Z

src/script/common/helper.cpp

+	return lua_toboolean(L, index) != 0;
+}
+
+template <> bool LuaHelper::readParam(lua_State *L, int index, const bool &dv)


clarity: dv -> default

default is a protected keyword in C++11 standard

also dv is described in the header comments

Well then default_ and fallback_value would still be an option

answers given

) * Modernize lua read (part 2 & 3): C++ templating assurance Implement the boolean reader Implement the string reader Also remove unused & unimplemented script_error_handler Add a reader with default value

Modernize lua read (part 2): C++ templating assurance

6d01189

Implement the boolean reader Also remove unused & unimplemented script_error_handler

nerzhul added @ Script API @ Client Script API Code quality labels Jun 4, 2018

nerzhul commented Jun 4, 2018

View reviewed changes

nerzhul added 3 commits June 5, 2018 00:33

LINT fix

f4146e3

Fix a detected regression in boolean code checks

baf9288

SmallJoker reviewed Jun 5, 2018

View reviewed changes

Fix @SmallJoker comments

045c33a

Add missing android entry

669a21d

nerzhul added 12 commits June 30, 2018 13:08

Revert a missed change

e5c01a1

Revert a missed change

2829aa1

Fix more regression cases

3e4c584

Last regression fix

ceead47

Make the API less strict

46f96c0

Revert "Last regression fix"

52bae7f

This reverts commit ceead47.

Revert "Fix more regression cases"

2215094

This reverts commit 3e4c584.

Revert "Revert a missed change"

467c2ea

This reverts commit 2829aa1.

Revert "Revert a missed change"

723b6ea

This reverts commit e5c01a1.

Revert "Fix a detected regression in boolean code checks"

1363239

This reverts commit baf9288.

Add a default values function to handle the regression cases properly

5465e25

Also added luaL_checkboolean to have a properly implemented boolean check

Drop luaL_checkboolean, it seems it has no sense for lua

97b1730

nerzhul changed the title ~~Modernize lua read (part 2): C++ templating assurance~~ Modernize lua read (part 2 & 3): C++ templating assurance Jun 30, 2018

sfan5 previously requested changes Jun 30, 2018

View reviewed changes

Make linter happy

11ec362

dv => default_value

0172c69

SmallJoker approved these changes Jun 30, 2018

View reviewed changes

LINT

4b135dc

nerzhul merged commit eef62c8 into minetest:master Jun 30, 2018

nerzhul deleted the modern_lua_read_2 branch June 30, 2018 15:11

rubenwardy added Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements and removed Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements labels Jul 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize lua read (part 2 & 3): C++ templating assurance #7410

Modernize lua read (part 2 & 3): C++ templating assurance #7410

nerzhul commented Jun 4, 2018 •

edited

Loading

nerzhul Jun 4, 2018

nerzhul commented Jun 5, 2018

nerzhul commented Jun 5, 2018

SmallJoker Jun 5, 2018

SmallJoker Jun 5, 2018

nerzhul Jun 5, 2018

SmallJoker Jun 5, 2018

nerzhul Jun 5, 2018

SmallJoker Jun 5, 2018

SmallJoker Jun 5, 2018

SmallJoker Jun 5, 2018

nerzhul Jun 5, 2018

paramat commented Jun 28, 2018

nerzhul commented Jun 28, 2018 •

edited

Loading

paramat commented Jun 28, 2018

nerzhul commented Jun 30, 2018

nerzhul commented Jun 30, 2018

sfan5 Jun 30, 2018

nerzhul Jun 30, 2018

sfan5 Jun 30, 2018

nerzhul Jun 30, 2018

nerzhul Jun 30, 2018

SmallJoker Jun 30, 2018

Modernize lua read (part 2 & 3): C++ templating assurance #7410

Modernize lua read (part 2 & 3): C++ templating assurance #7410

Conversation

nerzhul commented Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

nerzhul commented Jun 5, 2018

nerzhul commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paramat commented Jun 28, 2018

nerzhul commented Jun 28, 2018 • edited Loading

paramat commented Jun 28, 2018

nerzhul commented Jun 30, 2018

nerzhul commented Jun 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerzhul commented Jun 4, 2018 •

edited

Loading

nerzhul commented Jun 28, 2018 •

edited

Loading