[Discussion] getNumber and getBoolean defaultRet

[conde2]

What I would like to discuss here is if the function getNumber and getBoolean should return the default value when the argument passed through lua is nil. What is the impact of such change?

foo(param1, [param2, [param3]]) foo(1, nil, 3) -- in this case the default value will not be assigned to param2, instead it will assign 0

	template <typename T>
	static T getNumber(lua_State* L, int32_t arg, T defaultValue)
	{
		const auto parameters = lua_gettop(L);
		if (parameters == 0 || arg > parameters) {
			return defaultValue;
		}
		return getNumber<T>(L, arg);
	}

Solution

	template <typename T>
	static T getNumber(lua_State* L, int32_t arg, T defaultValue)
	{
		const auto parameters = lua_gettop(L);
		if (parameters == 0 || arg > parameters || lua_isnil(L, arg)) {
			return defaultValue;
		}
		return getNumber<T>(L, arg);
	}

https://github.com/otland/forgottenserver/blob/master/src/luascript.h#L280

[ranisalt]

I would rather implement that behavior in Lua.

What is the use case you have in mind?

[MillhioreBT]

If we put ourselves in that we would also have to verify if it is a table, function, string or boolean.

If someone used the string.format like this: string.format(nil, "name") maybe you should learn the basics of Lua, before trying to use the language, the TFS API is not strongly protected against human errors.

I am in favor of protecting ourselves from all kinds of errors, but it is true, adding many checks to getNumber could result in milliseconds of extra delays in most scripts.

You can work the default value from lua:

local n = tonumber(var) or 0
-------------------------------
foo(param1, [param2, [param3]])
foo(param1, tonumber(param2) or YOUR_DEFAULT_VALUE, param3) -- no sources changes required.

What about those that require that the check of is_table? A = the same as the previous code. is_function? is_userdata? is_thread? = A

See

print(tonumber(nil) or 0)
print(tonumber(0) or 0)
print(tonumber("text") or 0)
print(tonumber("10") or 0)
local co = coroutine.create(function()
    print(coroutine.status(co))
    coroutine.yield()
    print(coroutine.status(co))
end)
print(tonumber(co) or 0)
print(tonumber(tonumber) or 0)
--[[ OUTPUT:
0
0
0
10
0
0
]]

[conde2]

I would rather implement that behavior in Lua.

What is the use case you have in mind?

I'm implementing a HTTP Client in TFS, and I have a struct with default values for the HTTP Client and requests. My function receive a Lua table with the configs (similar what is done with outfits) and fills up this struct. I'm using the getField and if the field is nil then I use the defaultValue from my struct so I don't need to fill my Lua table with all config settings.

I will not bother answering the other guy above.

[ranisalt]

I understand. While it may make sense, since it wouldn't be beneficial to the code in this repo, I don't see the need to change - keep it if you feel it's worth it :)

However, as a suggestion, I would validate the input (and set whatever defaults you might need) in Lua before passing that table to C++.

[jopheno]

I totally disagree. It does make sense, since we can have the following example:

describe("bananinha", nil, "with small dots")

I am adding the nil there cuz I basically don't care to the second argument, but I do care for the third one. On CPP definition of describe it might have something like:

getString(L, 1) getNumber(L, 2, 1) getString(L, 3, "normal")

the idea is to return "1x bananinha with small dots", so in C++ it would do something that would look like this in LUA:

function describe(productName, count, specifier)
  return string.format("%sx %s %s", tostring(count), productName, specifier)
end

But... the result will be "0x bananinha with small dots", and this is totally error prone, cuz you are literally setting on getNumber for the default value to be 1, but NIL is turning into 0, NOT NIL, BUT ZERO.

Anyway...

(Edited the describe, which was just an example anyway...)

[conde2]

Even better would be checking if it is not a number, than it will avoid to check all other types and not default to zero which is incorrect and unexpected behaviour and instead default to the expected value defined in Cpp.

		template<typename T>
		static T getNumber(lua_State *L, int32_t arg, T defaultValue)
		{
			const auto parameters = lua_gettop(L);
			if (parameters == 0 || arg > parameters || !lua_isnumber(L, arg)) {
				return defaultValue;
			}
			return getNumber<T>(L, arg);
		}

I agree that it can be checked in Lua, as it can be done in many different ways, but because the default value is already defined in Cpp maybe this is a good approach.

[ranisalt]

I think it is defined in C++ mostly because what should we do if there is no value?

I would prefer, for example, returning an optional<T> and let the caller handle the absence of value.

[conde2]

I think it is defined in C++ mostly because what should we do if there is no value?

I would prefer, for example, returning an optional<T> and let the caller handle the absence of value.

Thats exactly the question, and you guys suggested to handle it in lua. However right now it is already handled in cpp but with some flaws that can be improved.

Using std::optional<T> can be a good solution though, could you give me an example how you are thinking in using it?

[MillhioreBT]

I totally disagree. It does make sense, since we can have the following example:

describe("bananinha", nil, "with small dots")

I am adding the nil there cuz I basically don't care to the second argument, but I do care for the third one. On CPP definition of describe it might have something like:

getString(L, 1) getNumber(L, 2, 1) getString(L, 3, "normal")

the idea is to return "1x bananinha with small dots", so in C++ it would do something that would look like this in LUA:

function describe(productName, count, specifier)
  return string.format("%dx %s %s", tostring(count), productName, specifier)
end

But... the result will be "0x bananinha with small dots", and this is totally error prone, cuz you are literally setting on getNumber for the default value to be 1, but NIL is turning into 0, NOT NIL, BUT ZERO.

Anyway...

First of all, your lua example is wrong, it would give the following error: bad argument #2 to 'format' (number expected, got string)

second you can handle whatever you want in lua and it's better:

int luaDescribe(lua_State* L)
{
	auto productName = getString(L, 1);
	auto count = getNumber(L, 2);
	auto specifier = getString(L, 3);

	pushString(L, fmt::format("{:d}x {:s} {:s}", productName, count, specifier));
	return 1;
}

local item = {
    name = "bananinha",
    count = nil, -- or any non-numeric data type.
    specifier = "with small dots"
}

local myString = describe(item.name, tonumber(item.count) or 1, item.specifier)

-- Result: 1x bananinha with small dots

if it bothers you, which would be weird... you can make an intermediate function so you don't have to write tonumber or 1,

do
    local oldDescribe = describe
    describe = function(name, count, specifier)
        return oldDescribe(name, tonumber(count) or 1, specifier)
    end
end

And that's it, you can now call describe without using tonumber or 1, since it will be done internally through the other intermediary function.

in case none convinces you, just use lua_isnumber(L, arg):

template < typename T>
static T getNumber(lua_State* L, int32_t arg, T defaultValue)
{
	if (lua_isnumber(L, arg) == 0) {
		return defaultValue;
	}

	return getNumber<T>(L, arg);
}

There is no need to measure the number of parameters on the stack, lua does that for you, and if it is not at a valid index for the stack it is the same as if there were a non-numeric value, that is, it will always return 0.

[conde2]

If we put ourselves in that we would also have to verify if it is a table, function, string or boolean.

If someone used the string.format like this: string.format(nil, "name") maybe you should learn the basics of Lua, before trying to use the language, the TFS API is not strongly protected against human errors.

I am in favor of protecting ourselves from all kinds of errors, but it is true, adding many checks to getNumber could result in milliseconds of extra delays in most scripts.

You can work the default value from lua:

local n = tonumber(var) or 0
-------------------------------
foo(param1, [param2, [param3]])
foo(param1, tonumber(param2) or YOUR_DEFAULT_VALUE, param3) -- no sources changes required.

What about those that require that the check of is_table? A = the same as the previous code. is_function? is_userdata? is_thread? = A

See

Nice to see that you have learned the basics of Lua and opened a pull request to fix this issue.

[jopheno]

but it makes particularly sense.

[ranisalt]

Using std::optional can be a good solution though, could you give me an example how you are thinking in using it?

	auto count = getNumber(L, 1);
	pushString(L, fmt::format("{:d}x", count.value_or(1));

[conde2]

Using std::optional can be a good solution though, could you give me an example how you are thinking in using it?

	auto count = getNumber(L, 1);
	pushString(L, fmt::format("{:d}x", count.value_or(1));

This could be a good option indeed, with this approach its possible to have multiple defined values for a missing parameter. However adds a little bit of overhead, for me would be fine leaving the way it is but fixing the issue with defaultRet.

[MillhioreBT]

If we put ourselves in that we would also have to verify if it is a table, function, string or boolean. If someone used the string.format like this: string.format(nil, "name") maybe you should learn the basics of Lua, before trying to use the language, the TFS API is not strongly protected against human errors. I am in favor of protecting ourselves from all kinds of errors, but it is true, adding many checks to getNumber could result in milliseconds of extra delays in most scripts. You can work the default value from lua:

local n = tonumber(var) or 0
-------------------------------
foo(param1, [param2, [param3]])
foo(param1, tonumber(param2) or YOUR_DEFAULT_VALUE, param3) -- no sources changes required.

What about those that require that the check of is_table? A = the same as the previous code. is_function? is_userdata? is_thread? = A See

Nice to see that you have learned the basics of Lua and opened a pull request to fix this issue.

maybe I was too hard on you, I'm sorry, there are usually a lot of trolls. And it doesn't bother me to accept my mistakes, thanks to that behavior I'm better every day.

and it was not my mistake, I know lua better than you can imagine, however you expressed yourself very badly in your examples, it is one thing to know if something is wrong and another thing is to know why something is wrong.