genericize `DataIdentity` for container types #4591

ab9rf · 2024-05-14T23:12:52Z

this partial rework of the data identity system provides genericized support for container types using concepts, and in general cleans up and modernizes this code

this doesn't cover several the std::vector specializations, still figuring out the best way to do that so this can be considered WIP but this code works on Windows and so i am putting up a PR mainly to run CI tests and see if gcc likes this or not

the process of going through this code has identified a number of other possible refinements that should probably be done which i'll come back to after i've had some rest

this partial rework of the data identity system provides genericized support for container types using concepts, and in general cleans up and modernizes this code this doesn't cover several the std::vector specializations, still figuring out the best way to do this

fix compliance with gcc-12 for `requires` (this is because gcc-10 implemented the concepts proposal while gcc-12 implemented the final standard which is different; msvc is lax and accepts either) also fix mistake in constraints for `isAssocContainer`

had the wrong type variable in the `container_identity` constructor, oops

this hides the underlying C++ type but this isn't necessarily useful anyway and exposing it requires complex nonportable code that is probably more work than it's worth

i'd like to not have to do this, but we're not there yet

lethosor · 2024-05-15T04:58:40Z

library/include/DataIdentity.h

+    template<isIndexedContainer C>
+    struct DFHACK_EXPORT identity_traits<C> {
+        static type_identity* get() {
+            static generic_container_identity<C> identity{};
+            return &identity;
+        }
+    };
+
+    template<isAssocContainer C>
+    struct DFHACK_EXPORT identity_traits<C> {
+        static type_identity* get() {
+            static generic_assoc_container_identity<C> identity{};
+            return &identity;
+        }
+    };
+
+    template<isSetContainer C>
+    struct DFHACK_EXPORT identity_traits<C> {
+        static type_identity* get() {
+            static generic_set_container_identity<C> identity{};
+            return &identity;
+        }
+    };
+


Are you sure these no longer need to be behind BUILD_DFHACK_LIB? My understanding is that had something to do with linking issues with plugins attempting to use these specializations, and it was deemed better/easier to not allow that at all, but I'm afraid I don't remember much more than that.

some context here: https://discord.com/channels/793331351645323264/807443262704058408/1239888219764690986

i'm still exploring this - i think it has to do inconsistent typing of the get static method. there appears to be no reason why all of the get methods cannot return a type_identity* as the type is polymorphic anyhow

however, i'm still investigating this

the issue of duplicate implementations still exists, but in this case the static object being created is read-only so if there are two copies there should be no risk of harm. the problem that arises when there are multiple instances of the same specialization is when there is mutable data, but there's no mutable data in a type_identity

also, both gcc and msvc have improved their loading models since 2012 to reduce problems with duplicated implementations

there is a ton of stuff in our code that is there to work around compiler bugs and shortcomings, or even C++ language shortcomings, that were fixed, in some cases, over a decade ago

the issue of duplicate implementations still exists, but in this case the static object being created is read-only so if there are two copies there should be no risk of harm.

Are we sure the address of that object isn't used as an identifier anywhere? I'm pretty sure at least something in the Lua layer passes around pointers to type_identity objects and compares them for equality.

At least some places where the pointer to type_identity is used:

Several registry tables:

The table at &DFHACK_TYPEID_TABLE_TOKEN maps type_identity* pointers (as raw lightuserdata) to the corresponding DF type object (e.g. identity_traits<df::item>::get() maps to the lua df.item).

The table at &DFHACK_TYPETABLE_TOKEN is a bidirectional map of type_identity* to metatable

In DF object metatables:

The value at &DFHACK_IDENTITY_FIELD_TOKEN is the object's type_identity. This is the value with a "userdata" key visible in printall(debug.getmetatable(df.item)), for example.

This is used in is_type_compatible(), but that already returns false for all IDTYPE_CONTAINER identities anyway, so your change has no effect there.

It's also returned by get_object_identity(), but that doesn't seem to be used for identification anywhere (although the result is passed on to several calls that I didn't trace...). Mainly, the identity is used by calling methods that should return the same thing for duplicate copies of the identity.

So, unless I'm missing something else, I think duplicates might be ok. Especially for templated container types, for which several operations that use the type_identity aren't implemented to begin with.

lethosor · 2024-05-15T04:58:59Z

library/include/DataIdentity.h

@@ -113,10 +114,10 @@ namespace DFHack

    class DFHACK_EXPORT container_identity : public constructed_identity {
        type_identity *item;
-        enum_identity *ienum;
+        type_identity *ienum;


Curious why this changed

specifically because of associative containers, the identity key type of an associative container goes there

Makes sense. I wonder if there is a way to enforce that this is an enum_identity for non-associative containers. Probably complicated.

mostly so this code will pass CI - in the long term i'd prefer something different

Bumber64 · 2024-05-16T20:48:08Z

Seems to work for me. I can pass a vector into Lua and modify it.

This reverts commit 93d9977.

due credit to james t.

library/include/DataIdentity.h

…ctor and deque

lethosor · 2024-05-19T06:05:14Z

library/include/DataIdentity.h

+        template<typename T> struct type_name<std::deque<T>> {
+            static const inline std::string name = "deque";
+        };
+        template<typename T, int sz> struct type_name<std::array<T, sz>> {


This needs to be

Suggested change

template<typename T, int sz> struct type_name<std::array<T, sz>> {

template<typename T, size_t sz> struct type_name<std::array<T, sz>> {

to match properly.

I am debating removing the name = "container" from the base template to catch this sort of thing.

Here's what I was testing with: DFHack/df-structures#757

I am debating removing the name = "container" from the base template to catch this sort of thing.

i was ambivalent on that issue originally. it's not hard for us to add a new specialization but it's more of a burden for a developer of an external plugin. maybe only include the generic case in the library build? that would cause any unspecialized in core code to error at compile time, while allowing plugin developers freedom to use specialized containers for their own purposes (although i'm not sure what the scenario for that is, tbh) nah i don't think that does what i want.. maybe someone else would have a better idea

Support for adding new types in plugins is limited anyway. Their identities won't be registered in the global registry tables in #4591 (comment) (although I'm a little unsure whether any container types are registered there).

Really, my intent of breaking the base template would be to catch specializations like array<T, int> that are intended to work but don't. It took me a while to figure out why a std::array was printing as "container" instead of "array".

fix parameter type of `std::array` Co-authored-by: Alan <lethosor@users.noreply.github.com>

Bumber64 · 2024-05-24T01:44:13Z

Regarding std::unordered_map, the keys (i.e., first) aren't accessible by Lua. Running pairs on it gives 0-n for keys and just the value of second for values (i.e., not {first, second}.)

Any attempt to insert into std:unordered_map<int, std::string> via Lua got me the error Temporaries of type string not supported. Same for std::unordered_set<std::string> and ordered versions.

ab9rf · 2024-05-24T02:08:58Z

Regarding std::unordered_map, the keys (i.e., first) aren't accessible by Lua. Running pairs on it gives 0-n for keys and just the value of second for values (i.e., not {first, second}.)

Any attempt to insert into std:unordered_map<int, std::string> via Lua got me the error Temporaries of type string not supported. Same for std::unordered_set<std::string> and ordered versions.

The Lua interface to C++ associative containers is at best shoehorned and needs to be reconceptualized. This PR may not be the best place to do this.

Bumber64 · 2024-05-24T02:12:29Z

The Lua interface to C++ associative containers is at best shoehorned and needs to be reconceptualized. This PR may not be the best place to do this.

Should I open an issue at this time? I don't need it for anything I'm working on. I just remembered it being mentioned on Discord as something to test. If nobody ever ends up needing it, then it'd be kind of pointless. (I guess Bay12 might use the type eventually?)

ab9rf · 2024-05-24T02:17:05Z

If nobody ever ends up needing it, then it'd be kind of pointless. (Bay12 might use the type eventually?)

Bay12 is already using std::maps in several places (notably in some of the interface-related stuff) and I seem to recall myk mentioning that having them fully available from lua would be useful in some places already.

Our current Lua interfaces only supports reading the "values" list as if it were a regular array, and does not support modifications at all. I personally believe that it would be good to move past these limitations.

ab9rf · 2024-05-24T02:19:39Z

Let me add here that this code is not really ready to go yet. I really want to extend this to encompass std::vector<T*> which right now is still being handled by the old stl_ptr_vector_identity, but this isn't just a matter of removing the latter specialization, changes also have to be made to the generic_container_identity for the special handling that vectors of pointers require.

I did test to see what happens if you just remove the specialization: the launcher sprouts hundreds of scrollbars, which is both hilarious and entirely unhelpful.

lethosor · 2024-05-25T04:52:20Z

Should I open an issue at this time? I don't need it for anything I'm working on. I just remembered it being mentioned on Discord as something to test.

Was that something I said? I didn't mean that unordered_map should have any particular functionality, just that this code should (a) compile and (b) not have any regressions when handling instances of unordered_map. The behavior you're pointing out is a known limitation that was present before this change. Not great, but not a regression.

myk002

does this deserve a changelog line (probably under API)?

ab9rf · 2024-05-27T15:24:41Z

does this deserve a changelog line (probably under API)?

lemme finish writing it first lol

lethosor · 2024-05-29T02:05:37Z

I'm not sure it does. It's really not a user-facing change. The only aspect that could affect some external devs is the ability to define std::arrays in df-structures, so I would probably put it under Structures, if anything.

ab9rf · 2024-05-29T11:44:04Z

the main "features" this PR offers are the ability to pass a wider range of types into Lua from C++, especially from plug-ins, future proofing the identity system to possibly support a broader range of container types (which we expect to be likely to arise as we continue to extend/revise structures), and general code quality (at least i would like to think so)

all of this is basically being driven by bay12's increasing use of C++20 paradigms that our identity system was never designed to work with. the existing system having was basically designed using C++03 + "boost but not really boost" which are at best dated by modern standards, but which worked very well with toady's C++98ish style of coding DF itself, and which doesn't necessarily play that well with C++20 which we are increasingly moving toward both because C++20 has some really nice paradigms (i am especially fond of std::range) but more because bay12 (with putnam now on board) is clearly moving rather quickly in that direction

i'm very much cognizant of the fact that we have, in recent revisions, added several types to structures that are currently being handled as opaque types within the Lua identity system but which really ought to have more meaningful Lua semantics. while this PR does not itself resolve this issue, i'd like to think that it's a step toward a more flexible identity management system that will hopefully enable us to do so. specific goals i have in mind here include making std::function objects directly callable from Lua (without explicit C++ glue code), allowing Lua code to manipulate the contents of C++ associative containers (including at the very least std::map, std::unordered_map, and std::set), and transparent support in Lua of types wrapped with std::optional or std::shared_pointer

anyway, i don't think any of this merits a changelog since it's 90% inside baseball and really only impacts the core team, at least initially. we didn't changelog #4406 as far as i know, and this is basically the same type of change as that was

ab9rf added 7 commits May 14, 2024 18:09

gcc-12 compliance

9701cf8

fix compliance with gcc-12 for `requires` (this is because gcc-10 implemented the concepts proposal while gcc-12 implemented the final standard which is different; msvc is lax and accepts either) also fix mistake in constraints for `isAssocContainer`

add omitted method to generic_container_identity

d7edcce

use the correct type variable

b6af192

had the wrong type variable in the `container_identity` constructor, oops

make getFullName less verbose

0c7dab2

this hides the underlying C++ type but this isn't necessarily useful anyway and exposing it requires complex nonportable code that is probably more work than it's worth

add insert requirement to isIndexedContainer

20d645c

make generic_assoc_container_identity read only

4aea762

i'd like to not have to do this, but we're not there yet

lethosor reviewed May 15, 2024

View reviewed changes

change "container" to "vector" in full name

93d9977

mostly so this code will pass CI - in the long term i'd prefer something different

ab9rf added 2 commits May 16, 2024 16:06

Revert "change "container" to "vector" in full name"

ed44b01

This reverts commit 93d9977.

just "change the conditions of the test"

95116d5

due credit to james t.

myk002 requested a review from lethosor May 16, 2024 22:24

lethosor reviewed May 17, 2024

View reviewed changes

library/include/DataIdentity.h Outdated Show resolved Hide resolved

lethosor reviewed May 17, 2024

View reviewed changes

library/include/DataIdentity.h Outdated Show resolved Hide resolved

lethosor and others added 3 commits May 17, 2024 06:25

generic_container_identity::getFullName(): return custom names for ve…

2b25eb6

…ctor and deque

add ResizableContainer concept

0afd6a9

do resizable containers correctly

bea43f6

lethosor reviewed May 19, 2024

View reviewed changes

lethosor mentioned this pull request May 19, 2024

Support stl-array DFHack/df-structures#757

Open

Update library/include/DataIdentity.h

7ce040b

fix parameter type of `std::array` Co-authored-by: Alan <lethosor@users.noreply.github.com>

Bumber64 mentioned this pull request May 24, 2024

Improve plants/plant plugin #4632

Open

myk002 reviewed May 27, 2024

View reviewed changes

support msvc 1940 (VS 17.10)

6944ca6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

genericize `DataIdentity` for container types #4591

genericize `DataIdentity` for container types #4591

ab9rf commented May 14, 2024

lethosor May 15, 2024

myk002 May 15, 2024

ab9rf May 15, 2024

ab9rf May 15, 2024

lethosor May 17, 2024

lethosor May 19, 2024

lethosor May 19, 2024

lethosor May 15, 2024

ab9rf May 15, 2024

lethosor May 17, 2024

Bumber64 commented May 16, 2024 •

edited

lethosor May 19, 2024

lethosor May 19, 2024

ab9rf May 19, 2024 •

edited

lethosor May 19, 2024 •

edited

Bumber64 commented May 24, 2024 •

edited

ab9rf commented May 24, 2024 •

edited

Bumber64 commented May 24, 2024 •

edited

ab9rf commented May 24, 2024

ab9rf commented May 24, 2024 •

edited

lethosor commented May 25, 2024

myk002 left a comment

ab9rf commented May 27, 2024 •

edited

lethosor commented May 29, 2024

ab9rf commented May 29, 2024

	template<typename T, int sz> struct type_name<std::array<T, sz>> {
	template<typename T, size_t sz> struct type_name<std::array<T, sz>> {

genericize DataIdentity for container types #4591

Are you sure you want to change the base?

genericize DataIdentity for container types #4591

Conversation

ab9rf commented May 14, 2024

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

Choose a reason for hiding this comment

Bumber64 commented May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ab9rf May 19, 2024 • edited

Choose a reason for hiding this comment

lethosor May 19, 2024 • edited

Choose a reason for hiding this comment

Bumber64 commented May 24, 2024 • edited

ab9rf commented May 24, 2024 • edited

Bumber64 commented May 24, 2024 • edited

ab9rf commented May 24, 2024

ab9rf commented May 24, 2024 • edited

lethosor commented May 25, 2024

myk002 left a comment

Choose a reason for hiding this comment

ab9rf commented May 27, 2024 • edited

lethosor commented May 29, 2024

ab9rf commented May 29, 2024

genericize `DataIdentity` for container types #4591

genericize `DataIdentity` for container types #4591

Bumber64 commented May 16, 2024 •

edited

ab9rf May 19, 2024 •

edited

lethosor May 19, 2024 •

edited

Bumber64 commented May 24, 2024 •

edited

ab9rf commented May 24, 2024 •

edited

Bumber64 commented May 24, 2024 •

edited

ab9rf commented May 24, 2024 •

edited

ab9rf commented May 27, 2024 •

edited