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 support for invocation and node iteration on pointer types #4

Merged
merged 2 commits into from
May 23, 2019

Conversation

Rogiel
Copy link

@Rogiel Rogiel commented Apr 26, 2019

This allows a meta::any or meta::handle to be invoked using the same syntax as a regular plain type. The conversion is performed automatically using type_node::ptr that converts a void* into a canonical form of T* for all types. handle now always stores the pointer in its canonical form.

What a type for a pointer does:

  • inherits ctor, dtor, func, data and prop from T
  • has it's own separate conv definitions (see note 2) although still not implemented, it should allow conversions from T* into T or T* into T** (this could be tricky since it might create an infinite recurring in template specializations).

What a handle for a pointer does:

  • Converts the void* into the canonical form and node is set to clazz (type T). Handles
  1. This DOES NOT support automatic conversion from T* to T for argument types as that should be implemented as a conv definition for the pointer types.
  2. T* and T conversions are not merged, since the rules for conversion of these types are different (a user-defined conversion operator that convert A -> B cannot convert A* -> B*).

@coveralls
Copy link

Pull Request Test Coverage Report for Build 42

  • 64 of 64 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 37: 0.0%
Covered Lines: 711
Relevant Lines: 711

💛 - Coveralls

@skypjack
Copy link
Owner

I'm out of home until Monday and looking at it from mobile, so forgive me if I'm doing obvious questions.
How does this for with the other PR?
I've still to fully review it because of some paid works that absorbed me last weeks. Moreover, it breaks some things and I wanted to know if it gets deprecated by this one before to proceed.

@Rogiel
Copy link
Author

Rogiel commented Apr 26, 2019

I'm out of home until Monday and looking at it from mobile, so forgive me if I'm doing obvious questions.
How does this for with the other PR?
I've still to fully review it because of some paid works that absorbed me last weeks. Moreover, it breaks some things and I wanted to know if it gets deprecated by this one before to proceed.

There's no hurry. Take your time with this.

This is independent from #3 but it should lay grounds for it to allow references to be invoked too.

In relation to the public API, there is no breaking change. In short:

  1. meta::resolve<T*>::data() should now return the same as meta::resolve<T>::data(). Previously it returned nothing and it was not possible to bind methods to it (due to factory now allowing pointer types). The same happens for all other accessors but conv.
  2. meta::type::clazz() has been added to the public API and currently returns the same value as would be returned by remove_pointer. When references (WIP: Add support for reference types on meta::any #3) are added the behavior will differ a bit (in relation to remove_pointer) by removing all attributes (const/volatile), * and & from the type. Essentially, returning the only type that can/should be bound with a factory.
  3. Pointer method invocation using the same syntax (and func/data) as you would when using a T (this is thanks to the canonical representation of the object).
// 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);

@Rogiel Rogiel mentioned this pull request May 12, 2019
@skypjack skypjack self-assigned this May 23, 2019
@skypjack skypjack added the enhancement New feature or request label May 23, 2019
@@ -743,6 +761,15 @@ TEST_F(Meta, MetaDtor) {
ASSERT_EQ(empty_type::counter, 0);
ASSERT_TRUE(dtor.invoke(empty));
ASSERT_EQ(empty_type::counter, 1);

dtor = meta::resolve<empty_type*>().dtor();
Copy link
Owner

Choose a reason for hiding this comment

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

I'm looking at this from GitHub, not fully tested yet (you know, daily jobs and the other things that steal time to the open source 😄).

The goal here is to get the meta type for empty_type when I invoke resolve<empty_type *>?
Therefore, pointers type are in a sense no more resolvable? I mean, there won't be any difference between T and T* from the point of view of the meta system when it comes to returning meta types?

Copy link
Author

@Rogiel Rogiel May 23, 2019

Choose a reason for hiding this comment

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

Not exactly. Their meta::types are different.

resolve<empty_type *>() != resolve<empty_type>()

But their "accessors" (ctor, dtor, func, etc...) are the same. So although their meta::type are different, the usage of these different types are very streamlined.

resolve<empty_type *>().func("my_func") == resolve<empty_type>().func("my_func")

If I want to check if a given type models the same "class" you need to check if their clazz is equal. The clazz is defined as always returning the bare user-defined type of a type. That is, the type without any "decoration" like const or pointer. This is the same type that you use when calling meta::factory methods.

resolve<empty_type *>().clazz() == resolve<empty_type>().clazz()

The func accessor for a pointer type behaves as-if:

meta::func meta::type::func(const char* name) {
  return clazz().func(name);
}

TLDR: If you want to find if a meta::any has an invokable foo, you don't have to care about it's concrete type, whether it's const, volatile, pointer or value. It will transparently for any type.

meta::any v = ...; // can be T or T*, doesn't matter
v.type().func("foo").invoke(v);
meta::resolve<T>().func("foo").invoke(v); // again, it doesn't matter if `v` is really a `T` here as long as it's `clazz()` is `T`
meta::resolve<T*>().func("foo").invoke(v); // same for `T*`

Copy link
Owner

@skypjack skypjack May 23, 2019

Choose a reason for hiding this comment

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

This is really clever indeed. Chapeau.

I want to rebase the PR on a dedicate branch where we can make all the changes required to support references as well. Most likely in the evening, when I've the time to work on the open source. 😉 Is it fine for you?
To be honest, on the other hand I'm not yet satisfied with #3 because of some things I've seen yesterday while looking into it. More details when I'm back home. In general, the way any manages references isn't exactly what I would expect from the library, but please give me the time to write everything with calm and in the right place.

As I said, I want to work on this because I find your proposal interesting. I also apologize for the late, but I've been very busy in the last weeks with some other projects and pending changes to close. 👍

Copy link
Author

Choose a reason for hiding this comment

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

A branch would be great. I was going to suggest that yesterday but forgot about it.

I am not completely satisfied with #3 as it is either, maybe with the branch we can work it out better.

Just ping me whenever you got the branch ready.

Copy link
Owner

Choose a reason for hiding this comment

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

I've merged it. However, it fails on appveyor. 😢

Copy link
Owner

@skypjack skypjack May 24, 2019

Choose a reason for hiding this comment

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

Yeah, it's apparently vs2015 that goes crazy with this guard. 😢
I'll make some other attempts in the afternoon, let me know if you find a work around for it in the meantime.

It seems that it tries to substitute T in the expression within the if constexpr even if the guard is false. Because of that, it goes without saying that there are cases where it doesn't produce valid code. As an example, when T is int, you cannot obviously convert an int * to auto **. The errors are almost always along this line.

Copy link
Author

Choose a reason for hiding this comment

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

VS2017 if constexpr was pretty broken from what I remember, it sometimes would try to evaluate the false branch and (rightfully) fails to compile. It was hit and miss though.

Maybe we'll need to either drop VS2017 or use templates classes and specializations to deal with this the old way. Gotta say, I hate working with MSVC, it is getting better over time, but damn, it is still annoying to work with.

I am pretty sure function pointers are be convertible to void*. In both Windows and POSIX dlsym (and the equivalent for Win32) returns a void* that points to a function in the dynamic library. So unless both POSIX and Win32 are both relying on UB here, I am pretty sure that is fine in this case.

The real problem is that member pointers are usually implemented as an offset relative to the object absolute memory address (and maybe the vtable) and thus might or might not be the same size as a pointer. Function pointers through don't have this problem and are guaranteed to be the same length as a pointer and convertible to a void*.

I will see if I can find a solution for that in the weekend. I already installed MSVC 19.16 to test this using the same compiler in appveyor.

Copy link
Author

Choose a reason for hiding this comment

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

I mean this, even though VS doesn't like it as well. Let me check into the error.

Wouldn't a !std::is_member_object_pointer_v and !is_member_function_pointer be easier and clearer?

Copy link
Owner

Choose a reason for hiding this comment

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

Hi @Rogiel - have you found the time to look into this?

Copy link
Author

Choose a reason for hiding this comment

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

No, not yet. Sorry.

@skypjack skypjack changed the base branch from master to experimental May 23, 2019 19:31
@skypjack skypjack merged commit e2f565f into skypjack:experimental May 23, 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.

3 participants