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

WIP: Add support for reference types on meta::any #3

Closed
wants to merge 2 commits into from

Conversation

Rogiel
Copy link

@Rogiel Rogiel commented Apr 24, 2019

Hello @skypjack, this is a possible implementation for #2. This is very WIP and breaks a lot of things.

  • Fix broken existing tests (some are now broken, haven't looked deep into why yet)
  • Implement casting from T& to const T&
  • Implement conversion from T& and const T& to T
  • Implement more tests
  • How to handle functions that return a reference, should it default to a meta::any holding a reference?
  • Remove duplicated code

@Rogiel
Copy link
Author

Rogiel commented Apr 24, 2019

I think renaming remove_pointer into remove_attributes or base_type internally would be good. Since now it also removes &. Could also be used to remove const and volatile attributes.

@Rogiel
Copy link
Author

Rogiel commented Apr 24, 2019

I have fixed all tests, but I had to change some of them. Now the library is taking into account the const-ness of arguments and whether it is a reference or not.

Sadly, that is probably a breaking change for existing code :(

@coveralls
Copy link

Pull Request Test Coverage Report for Build 40

  • 82 of 92 (89.13%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.3%) to 98.728%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/meta/factory.hpp 12 16 75.0%
src/meta/meta.hpp 70 76 92.11%
Totals Coverage Status
Change from base Build 37: -1.3%
Covered Lines: 776
Relevant Lines: 786

💛 - Coveralls

@skypjack skypjack self-assigned this Apr 24, 2019
@skypjack skypjack added the enhancement New feature or request label Apr 24, 2019
@Rogiel Rogiel mentioned this pull request May 12, 2019
@@ -851,7 +955,7 @@ TEST_F(Meta, MetaDataConstStatic) {

ASSERT_TRUE(data);
ASSERT_EQ(data.parent(), meta::resolve("data"));
ASSERT_EQ(data.type(), meta::resolve<int>());
ASSERT_EQ(data.type(), meta::resolve<const int>());
Copy link
Owner

@skypjack skypjack May 22, 2019

Choose a reason for hiding this comment

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

Finally, I got a pause from other projects and I want to dedicate some spare time to meta so as to add full support for references. I find it useful to move back and forth big types actually, so I think it's worth it.
Unfortunately, I tried this change on a software of mine and found that it added as much as it removed. In particular, the fact that a const int data member is no longer identified as int from the meta system breaks some code my side. Similarly, the same applies to other types and different qualifiers.

From my point of view, the opacity of the meta system with regards to types was a value, not something to be defeated.
Imagine an editor that works with meta types. Where I had to deal only with int, double and so on, now I've to deal with int, const int, double, const double, and so on. Not really an improvement.

Isn't there a way to do that à la rttr, using std::reference_wrappers where explicitly required?
During the last days I thought also to something more C++17-ish, where I can specify a function type or a data type aside the actual member. As an example:

meta::reflect<my_type>("T").data<&my_type::i, std::reference_wrapper>("i");
// as well as
meta::reflect<my_type>("T").data<&my_type::i, double>("i");

In other terms, a way to tell to the system - ok, this is the data member, no matter what its type is, return it to me using the given type.
This could work for data members as well as functions, constructors and so on, other than for pointer-to-reference conversions under the hood when you say - here is a pointer, return it as a reference, please.
Moreover, if I think to the example above with the editor, it would be good to further reduce the types with which to work by doing something borrowed from js, that is a throughout conversion of all numbers to double.

What do you think about?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, the problem with this PR is exactly that it is not quite backwards compatible as it was. But after #4 it should be able to work in most cases as-is, since we can make T& funcs iterable too:

// syntax might not be 100% accurate since i did this from the top of my head
auto r = func.invoke(...); // returns a T&
r.type().data("value").value(r); // should work with T, T& or T* (#4)

I think it is important to know the const-ness information, since the behaviour of a const object is a bit different than a non-const in some situations. Otherwise we would have to always perform a const_cast (which is undefined behaviour in some instances, see below). We would also say goodbye to any opportunity of overload resolution.

static const int n = 0;

struct C {
  const int& n() { return n; }
  void test(int& n) { n++ };
}

meta::func func_n = ...;
meta::func func_test = ...;

meta::any c = c; // is of type C
meta::any r = func_n.invoke(c);
func_test.invoke(c, r);

This (the last invoke) is UB, since a const_cast will need to be performed implicitly and the standard allows the compiler to allocate the global n in a read-only section in the executable. In the optimal case things crash. We also cannot do any implicit copy because we don't know if it is const or not.

I dislike the rttr approach with regards to references. Simply because we can (and IMO should) use the language itself to to deduce the return type. If the function returns a reference, I would like a reference to the returned object. In the (unlikely) case that I want a value, only then I should request it explicitly. Even better would be something like:

auto ret = f.invoke(...); // returns a &
ret.to_value(); // explicitly copies the object -- could be simply a call to ret.conv<T>() since (const) T& is convertible to a value

This approach would also make more difficult to use Clang-based tools on the source code and generate bindings automatically, because then we need to provide extra context to the parser.

Also, I think part of what you are proposing at the end is that #4 does, but with pointer types instead. I think we should start there first, since that change is fully backwards compatible and will significantly improve this PR too.

@@ -960,7 +1064,7 @@ TEST_F(Meta, MetaDataSetterGetterWithRefAsMemberFunctions) {
ASSERT_TRUE(data);
ASSERT_NE(data, meta::data{});
ASSERT_EQ(data.parent(), meta::resolve("setter_getter"));
ASSERT_EQ(data.type(), meta::resolve<int>());
ASSERT_EQ(data.type(), meta::resolve<const int&>());
Copy link
Owner

Choose a reason for hiding this comment

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

Here is another example of what I said before. This change introduced a sort of (let me say) explosion of types to take in consideration when most of the times I use this system with trivial data structures or simple functions. I don't really want to care of the fact that an argument is a const int & instead of an int. As long as the meta ctor returns me the int meta type for it I'm fine in 99% of the times. In this case I've to take in consideration the whole complexity of the type system of the C++, that was exactly what I tried to hide behind a simpler layer.

I agree that support for references is a must-have, but I'd like to find a way that doesn't turn meta in a tool with a type system as complex as the language it abstracts.

Copy link
Author

Choose a reason for hiding this comment

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

As I said in the previous comment, #4 should address most of these issues. You can that that directly now. Just checkout #4 and use pointer types instead.

You will see that you don't actually have to care about const-ness (const is not supported there, but still the concept should still apply) or if a object is a pointer. You just give it as an argument and things just work.

Copying the example I gave there for reference:

// Note 1: Pointer types now have their members iterable
meta::func f = meta::resolve<T>()::func("test");
meta::func f2 = meta::resolve<T*>()::func("test"); // is also valid and is the same as `f`
assert(f == f2);

// Note 2: `clazz` removes `*`
assert(meta::resolve<T*>().clazz() == meta::resolve<T>());

// Note 3: both should work the same and does the correct thing, either with `handle` or `any`
T* object = ...;
f.invoke(object);
f.invoke(*object);

Also, importantly in the first note, both f1 or f2 are actually the same object (extracted from the clazz of type T). When using as an argument or as a this (first argument of invoke), you don't care if it is a pointer or not. It will just work:

// the "pseudo"-argument of invoke represents the type contained in an `any`.
f1.invoke(T*) // works
f2.invoke(T*) // works
f1.invoke(T) // works
f2.invoke(T) // works

Copy link
Author

Choose a reason for hiding this comment

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

Also of note:

ASSERT_EQ(data.type().clazz(), meta::resolve<int>());

clazz() can be used to return the same value as would be returned previously. This is a breaking change still, but eases the pain of it.

@skypjack
Copy link
Owner

skypjack commented Jul 6, 2019

I'm closing this because it's irremediably in conflict with master. I'm interested in the proposed changes, but the PR cannot be accepted as it stands unfortunately.

@skypjack skypjack closed this Jul 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants