Skip to content

remove lua.h dependency from vtableinterpose.h #5628

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
+23 −5
Open

remove lua.h dependency from vtableinterpose.h #5628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions library/DataIdentity.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

#include "DataFuncs.h"
#include "DataIdentity.h"
#include "LuaWrapper.h"

// the space after the uses of "type" in OPAQUE_IDENTITY_TRAITS_NAME is _required_
// without it the macro generates a syntax error when type is a template specification
Expand Down Expand Up @@ -62,4 +63,19 @@ namespace df {

const stl_container_identity<std::vector<int32_t> > stl_vector_int32_t_identity("vector", identity_traits<int32_t>::get());
const stl_container_identity<std::vector<int16_t> > stl_vector_int16_t_identity("vector", identity_traits<int16_t>::get());

type_identity* lua_touserdata_(lua_State* state)
{
return (type_identity*)lua_touserdata(state, DFHack::LuaWrapper::UPVAL_ITEM_ID);
}

void* get_object_internal_(lua_State* state, type_identity* id, int val_index, bool val)
{
return DFHack::LuaWrapper::get_object_internal(state, id, val_index, val);
}

void field_error_(lua_State *state, int index, const char* err, const char* mode)
{
DFHack::LuaWrapper::field_error(state, index, err, mode);
}
Copy link
Member

@ab9rf ab9rf Nov 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These introduce potential pessimizations by preventing the compiler from inlining these at their call sites. Since at least two of these functions are used in potentially high-velocity code paths, I'm reluctant to approve this without runtime profiling

that said, i just tested dfhack compiled with LTCG and it seems to work on Windows, which would obviate this concern if we can confirm that using LTCG doesn't break dfhack, and someone can figure out how to do the same thing for linux users

}
12 changes: 7 additions & 5 deletions library/include/DataIdentity.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,9 @@ distribution.
#include <unordered_map>
#include <variant>
#include <vector>
#include <variant>
#include <filesystem>

#include "DataDefs.h"
#include "LuaWrapper.h"

namespace std {
class condition_variable;
Expand Down Expand Up @@ -393,6 +391,10 @@ namespace df
};
#endif

type_identity* lua_touserdata_(lua_State* state);
Copy link
Member

@lethosor lethosor Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are the trailing underscores necessary? can we not just fully-qualify the names when needed (or would that require changing too many places)?

Copy link
Contributor Author

@dhthwy dhthwy Nov 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are forwarding functions (intended for DataIdentity only) used by DataIdentity.c so that we can include LuaWrapper.h which in turn includes lua.h in the DataIdentity TU instead of the header.

the real lua_touserdata() takes another argument too.

_wrapper or something similar is another alternative. I wasn't sure what name to give it, hence the underscores.

I don't think they're necessary?

void* get_object_internal_(lua_State* state, type_identity*, int val_index, bool);
[[noreturn]] void field_error_(lua_State *state, int index, const char* err, const char* mode);

template<class T>
class stl_container_identity : public container_identity {
const char *name;
Expand Down Expand Up @@ -424,8 +426,8 @@ namespace df
{
using VT = typename T::value_type;
VT tmp{};
auto id = (type_identity*)lua_touserdata(state, DFHack::LuaWrapper::UPVAL_ITEM_ID);
auto pitem = DFHack::LuaWrapper::get_object_internal(state, id, val_index, false);
auto id = lua_touserdata_(state);
auto pitem = get_object_internal_(state, id, val_index, false);
bool useTemporary = (!pitem && id->isPrimitive());

if (useTemporary)
Expand All @@ -435,7 +437,7 @@ namespace df
}

if (id != item || !pitem)
DFHack::LuaWrapper::field_error(state, fname_idx, "incompatible object type", "insert");
field_error_(state, fname_idx, "incompatible object type", "insert");

return insert(ptr, idx, pitem);
}
Expand Down