Make code more explicit and safe

[mchoo7]

• Replaced NULL with nullptr
• Removed void in function declaration/definition
• Added include guard
• Replaced virtual with override and final when possible
• Mark some functions const
• Removed some redundant code
• Replaced deprecated syntax such as throw() and auto_ptr
• Use static_cast<> for type conversion

[ngie-eign]

These are a lot of changes. Could you please break them down by group and run the tests (at the very least on a recent version of Linux)? There’s a good chance we’ll need to do “mop-up” later fixing things on other platforms, but I’d like to review your changes quickly/efficiently and is sensible batches. The less changes, the more efficiently I can review, then merge them.

[mchoo7]

lutok/state_test.cpp

Lines 797 to 807 in 4b39412

	ATF_TEST_CASE_WITHOUT_HEAD(push_cxx_function__fail_anything);
	ATF_TEST_CASE_BODY(push_cxx_function__fail_anything)
	{
	lutok::state state;
	state.push_cxx_function(cxx_divide);
	lua_setglobal(raw(state), "cxx_divide");
	ATF_REQUIRE(luaL_dostring(raw(state), "return cxx_divide(-3, -1)") != 0);
	ATF_REQUIRE_MATCH("Unhandled exception", lua_tostring(raw(state), -1));
	lua_pop(raw(state), 1);
	}

In this case, should it throw an exception:

lutok/state_test.cpp

Lines 153 to 165 in 4b39412

	static int
	cxx_divide(lutok::state& state)
	{
	const int dividend = state.to_integer(-2);
	const int divisor = state.to_integer(-1);
	if (divisor == 0)
	throw std::runtime_error("Divisor is 0");
	if (dividend < 0 || divisor < 0)
	throw std::string("Cannot divide negative numbers");
	state.push_integer(dividend / divisor);
	state.push_integer(dividend % divisor);
	return 2;
	}

Other tests pass without any problem.

[mchoo7]

Fixed the test for now. Please notify me if there is any problem with this fix.

[ngie-eign]

I can only guess why @jmmv wrote it this way, but in K&R C (which predates C++), the () form vs the (void) form can be problematic: https://stackoverflow.com/a/7412301 . I honestly think it's best to be explicit -- it gets programmers into a habit of being explicit, which (given ATF) is important as it can introduce bugs in C.

[ngie-eign]

Remove redundant qualifiers

I personally believe removing namespace qualifiers like this can prove itself more problematic in the long run, much like star imports in python. It makes it difficult to understand where code lives and can result in accidental namespace collisions.

Using better to be explicit and keep the namespace qualifiers.

[ngie-eign]

Remove redundant static definition

Again, I can only guess @jmmv 's original intentions, but using static in C code at least can result in more optimal code with -O0 and can avoid namespace collisions and function/variable shadowing.
It's better to leave static in IMHO to be explicit and to avoid these kinds of issues, for the reasoning I provided in my last comment.

[ngie-eign]

Replace <cstring> with <string>

NAK: std::strcmp (which is used in this source file) is exposed via cstring, not string: https://en.cppreference.com/w/cpp/string/byte/strcmp .

[ngie-eign]

Do not include unused headers

NAK:

• lua.hpp is used for definitions like lua_State. It's best to be explicit, not implicit.
• lutok/state.ipp: unsure if this is actually used.
• cstring: this appears to be unused.

[ngie-eign]

Use std::move()

NAK:
This code seems like it would be less performant now with -O0 since it's passing by value instead of by reference. Also, depending on how it's called it could result in more data copies on input.
I think the former form is better: it's more explicit and less "by side-effect" than the new form.

[mchoo7]

@ngie-eign I tried to build on macOS 15, but I get this error while running ./configure

./configure: line 19249: syntax error near unexpected token `ATF_CXX,'
./configure: line 19249: `        PKG_CHECK_MODULES(ATF_CXX, ${spec},'

Is this a bug?

[ngie-eign]

@ngie-eign I tried to build on macOS 15, but I get this error while running ./configure

./configure: line 19249: syntax error near unexpected token `ATF_CXX,'
./configure: line 19249: `        PKG_CHECK_MODULES(ATF_CXX, ${spec},'

Is this a bug?

Did you run make distclean first or admin/clean-all.sh?

[mchoo7]

@ngie-eign I tried to build on macOS 15, but I get this error while running ./configure

./configure: line 19249: syntax error near unexpected token `ATF_CXX,'
./configure: line 19249: `        PKG_CHECK_MODULES(ATF_CXX, ${spec},'

Is this a bug?

Did you run make distclean first or admin/clean-all.sh?

Sorry my bad. I didn't install lua which was the issue. Build success on macOS 15.

[mchoo7]

I'm trying to test on FreeBSD, but ./configure cannot find lua5.4.

pkgconf and lua54 are installed.

checking for pkg-config... /usr/local/bin/pkg-config
checking pkg-config is at least version 0.9.0... yes
configure: ATF support disabled
checking for doxygen... no
checking for lua5.4 >= 5.4... no
configure: error: Package requirements (lua5.4 >= 5.4) were not met:

Package 'lua5.4' not found

Consider adjusting the PKG_CONFIG_PATH environment variable if you
installed software in a non-standard prefix.

Alternatively, you may set the environment variables LUA_CFLAGS
and LUA_LIBS to avoid the need to call pkg-config.
See the pkg-config man page for more details.