pmed / v8pp

Bind C++ functions and classes into V8 JavaScript engine
http://pmed.github.io/v8pp/
Other
898 stars 120 forks source link

Allow property getter and setter to be overloads by matching const #51

Closed tim-janik closed 5 years ago

tim-janik commented 7 years ago

Allow property getter and setter to be overloads by matching const method.

This is usefull in case the property getter and setter have the same name, example:

  double  bpm () const;
  void    bpm (double);

Signed-off-by: Tim Janik timj@gnu.org

pmed commented 7 years ago

There is already property(Get, Set) function template that is suitable for any pair of get/set member functions, including overloaded ones. But there is a quirk in C++, and a compiler can't deduce types of argument in property(&SomeClass::bpm, &&SomeClass::bpm) because they are ambiguous.

So you have to specify actual type for each pointer to the overloaded functions. For you sample such types will be:

using getter = double (SomeClass:**)() const;
using setter = void (SomeClass:**)(double);

// Here we set actual types of pointers to the functions
v8pp::property(getter(&SomeClass::bpm), setter(&SomeClass::bpm));

I've added a test for such a case: 142de1e2aa0fb2493dc40b7532fece15124010bf

tim-janik commented 7 years ago

I can see how that fixes the brief test case given. But I'm using v8pp for auto.generated bindings, adding explicit argument and return types makes the code I'm using much more complicated. Just to give you an idea, here's the relevant line from the current branch: https://github.com/tim-janik/beast/blob/e8e69fcd61f5db949303b0a47f74dc6d85e7db16/ebeast/v8bse/V8Stub.py#L151

Is there anything wrong with the approach to match on const this? AFAIK, that's a common C++11 idiom, but if there's something fundamentally flawed I could try to fix PR#51. Or I could resort to have the specialisation in my code only, but I'd still like to know if you see any issues with it, like upcoming incompatibilities or somesuch.

pmed commented 7 years ago

Your property() overloading is suitable only for one particular pair of get/set signatures, but there may be other variations like:

double bpm() &&;
bool bpm(v8::Isolate*, int);

I think that current generic version of v8pp::property(Get, Set) function is suitable for all such cases, but another overloading with the same number of arguments might cause ambiguity.

So I'd prefer to have only this generic function in the library code. Anyway, as you mentioned, it's always possible to have another overloading in your code.

tim-janik commented 5 years ago

Anyway, as you mentioned, it's always possible to have another overloading in your code.

Thanks for your response. After v1.6.0, I've reworked this into a helper indeed. For the record:

/// Create read/write property from get and set member functions, match getter via const this
template<class Class, typename Ret, typename Arg>
v8pp::property_<Ret (Class::*) () const, void (Class::*) (Arg)>
v8pp_property (Ret (Class::*get) () const, void (Class::*set) (Arg))
{
  v8pp::property_<Ret (Class::*) () const, void (Class::*) (Arg)> prop (get, set);
  return prop;
}
// Usage:
class MyObject {
  std::string name () const;
  void        name (const std::string &newname);
};
class_.set ("name", v8pp_property (&MyObject::name, &MyObject::name));