Skip to content
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.

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

add property setter function that does not depend on getter #271

Closed

Conversation

rpatters1
Copy link
Contributor

This pull request implements a separate addPropertySetter function for Class. If you use a reflection library like refl-cpp to load your classes into LuaBridge, it is inefficient or even impossible (due to Visual Studio 2019 limitations) to know the property getter and setter at the same time. This simple new function resolves the issue.

It would have been possible to overload addProperty, but that had the disadvantage of making it far too easy to add a setter instead of a getter by mistake (or vice versa). So I opted to give it a different function name.

Resolves #265

@dmitry-t
Copy link
Collaborator

dmitry-t commented Nov 27, 2021

Sorry, I'm not familiar with the library you mentioned.
So is there a limitation on 1) adding a getter and then 2) adding a getter and a setter (where a new getter replaces the older one)?
Because the protocol (not the implementation of course) you propose is quite complicated.

@rpatters1
Copy link
Contributor Author

rpatters1 commented Nov 27, 2021

Here's the documentation for refl-cpp.

A very brief reflection example might be something like:

class MyType
{
   int value;

public:
   MyType : value(0) {}
   int GetValue() const { return value; }
   void SetValue(int newval) { value = newval; }
   int SomeOtherFunction(std::string &text);
};

I have written a (partially) compile-time AddClass function that adds any reflected class to LuaBridge. Here is a simplified version of it:

#include "refl.hpp"

template<class T, typename TN>
void AddClassMembers(TN& bridgeClass)
{
   constexpr auto type = refl::reflect<T>();
   constexpr auto members = get_members(type);

   for_each(members, [&](auto member)
   {
      bridgeClass.addFunction(refl::descriptor::get_name(member).c_str(), refl::descriptor::get_pointer(member));
      if constexpr (refl::descriptor::is_property(member))
      {
         if constexpr (refl::descriptor::is_writable(member))
         {
            bridgeClass.addPropertySetter(refl::descriptor::get_display_name(member), refl::descriptor::get_pointer(member));
         }
         else
         {
            bridgeClass.addProperty(refl::descriptor::get_display_name(member), refl::descriptor::get_pointer(member));
         }
      }
   });
}

template<class T>
void AddClass(const std::string& nameSpace, lua_State *l)
{
   constexpr auto type = refl::reflect<T>();
   
   auto bridgeClass = luabridge::getGlobalNamespace(l)
                        .beginNamespace (nameSpace.c_str())
                           .beginClass<T>(refl::descriptor::get_name(type).c_str());

   AddClassMembers<T>(bridgeClass);

   bridgeClass.endClass()
      .endNamespace();
}
.
.
.
// and here I actually add the class to LuaBridge
// first, the REFL_AUTO macro that defines the reflected members
REFL_AUTO (
   type(MyType),
   func(GetValue, property()),
   func(SetValue, property()),
   func(SomeOtherFunction)
)
// then add the class
AddClass<MyClass>(namespace_str, l);

Originally I created a second list of methods to search for property setters. It required two nested levels deep of lambda functions. Although it worked in XCode, it was cumbersome and difficult to read. But it failed entirely in Visual Studio because (due possibly to a bug), Visual Studio will not pass lambda captures 2 nested levels deep in a constexpr loop.

Adding a separate addPropertySetter function made this code a breeze to understand, and it worked on both platforms I'm dealing with. I am currently loading a massive class library into LuaBridge using addPropertySetter for the property setters, and it works like a dream. The only possible downside I can see is that it would permit write-only properties, though my code doesn't have any. But I don't see any problem with write-only properties in principle. I've occasionally actually used them in other contexts, though hesitantly of course.

@rpatters1
Copy link
Contributor Author

Also, I should add that in my code the property setters and getters come higgledy-piggledy in any order, but ultimately they all get added correctly.

@rpatters1
Copy link
Contributor Author

rpatters1 commented Nov 27, 2021

Also, it seems to me that one would either use

addProperty(name, getter, setter);

or

addPropertySetter(name, setter);
addProperty(name, getter);
// in either order, which currently works with my pr

But not both. Does LuaBridge now permit adding properties twice?

@rpatters1
Copy link
Contributor Author

rpatters1 commented Dec 7, 2021

I have actually encountered write-only properties in the wild. I'm not saying it's something I would ever implement, but I have seen them. A recent example where I might conceivably have implemented one is on a list-control class I was working on. There is an option on macOS to have alternating colors on the background. I could conceive of a write-only property to set an option like that. There is basically no reason to ever interrogate it but every reason to set it true or false. (I ended up implementing it as a method rather than a property, but I did not provide a way to interrogate it.)

To be clear, I am not proposing this pull request to have write-only properties. That's just a harmless side-effect. Answering your second question, member in refl::descriptor::get_pointer(member) is a function, not a variable. So the solution you proposed does not work. Referring back to the simplified reflection-list macro in my example:

REFL_AUTO (
   type(MyType),
   func(GetValue, property()),
   func(SetValue, property()),
   func(SomeOtherFunction)
)

Note that every member is func. I have several hundred classes, each with anywhere from a dozen to a couple of hundred functions being bound thru LuaBridge with lists like these. The order they are listed in is pretty much random. I need to be able to enter the property setters when I encounter them and the property getters when I encounter them, without having to know both at the same time. That's the reason for the pr.

To further clarify, when the member is a function, refl::descriptor::is_writable(member) is false if the function is const and true otherwise.

@dmitry-t
Copy link
Collaborator

dmitry-t commented Dec 7, 2021

So I took a look at the refl-spp.
I think you refl::descriptor::get_reader() is what you need.

         if constexpr (refl::descriptor::is_writable(member))
         {
             bridgeClass.addProperty(
                 refl::descriptor::get_display_name(member),
                 refl::descriptor::get_pointer(refl::descriptor::get_reader(member)),
                 refl::descriptor::get_pointer(member));
         }
         else

You only should carefully handle the else case in order to avoid registering the already registered property.

@rpatters1
Copy link
Contributor Author

*mind-blown emoji*

Actually, what I need is get_writer, because there are plenty of read-only properties in my code. This may indeed be the ticket. If I can get it to work, I'll close the pr. If not, I'll report back.

Thank you for digging that out.

@rpatters1
Copy link
Contributor Author

I ended up using get_reader after all, as you suggested, along with has_writer. It ferreted out some accidental write-only properties, so even better. It works, but it greatly extends the compile time by close to an order of magnitude. I suppose this was to be expected. Reflection is going to be ridiculously inefficient in compilers at least until someday when the C++ Standard includes it built-in. The runtime code seems to be fast enough, though.

The puny POS 32-bit compiler in Visual Studio 2019 just can't eat my largest REFL lists when I do it this way. It runs out of heap space. In a rare sequence of fortunate events, Microsoft just released Visual Studio 2022 a few weeks ago. It finally is a 64-bit platform, and it can build my project.

I may still use my fork with addPropertySetter for speed, perhaps until I can get a faster dev machine. But at least now I know how to get back on the official master branch. Thanks. I'm closing this pr.

@rpatters1 rpatters1 closed this Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding property setters separately
2 participants