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

This library could support more types than just aggregates #64

Open
hanslhansl opened this issue Oct 26, 2024 · 0 comments
Open

This library could support more types than just aggregates #64

hanslhansl opened this issue Oct 26, 2024 · 0 comments

Comments

@hanslhansl
Copy link

Most of reflect's functions and typedefs are restrained by std::is_aggregate_v<T>. I believe this constraint to be unsuitable and either too lax or too strict, depending on the intended behaviour.

1. std::is_aggregate

According to cppreference in c++20 an aggregate is either an array type or a class type that has:

  1. no user-declared or inherited constructors
  2. no private or protected direct non-static data members
  3. no virtual base classes
  4. no private or protected direct base classes
  5. no virtual member functions

2. Type and value related features

For type and value related features (e.g. reflect::member_type<N, T>, reflect::visit(), reflect::get()) my reasoning is a bit simpler so lets start with those:
As far as I understand all of these features boil down to a structured binding declaration like auto [identifiers] = e. The binding is performed in one of three ways depending on the type E of e:

  1. E is an array type. The introduced identifiers bind to the array elements.
    This library mirrors this behaviour. reflect::member_type<I, A[N]> is A and reflect::get<I, A[N]>(arr) returns a reference to the Ith element of arr. Expected and actual behaviour are identical.

  2. E is a non-union class type which supports the "tuple-like" binding protocol. The introduced identifiers bind to e.get<I>() or get<I>(e) and are of type "reference to std::tuple_element<I, E>::type". Note that e.get<I>()/get<I>(e) do not necessarily return a reference to the Ith member of e and std::tuple_element<I, E>::type is not necessarily the same as the type of the Ith member of e.
    If E is an aggregate type reflect mirrors this behaviour because it relies on structured bindings. This isn't the expected behaviour though. The user expects reflect::member_type<I, T> to be the type of the Ith member and reflect::get<I, T>() to return the Ith member irrespective of whether T supports the "tuple-like" binding protocol. Especially in projects that use a lot of meta programming this behaviour might go unchecked and introduce hard to find bugs.
    If E isn't an aggregate type reflect fails to compile. This at least won't introduce any bugs.

  3. E is a non-union class type which doesn't support the "tuple-like" binding protocol. The introduced identifiers bind to the non-static data members of e.
    If E is an aggregate type reflect mirrors this behaviour. reflect::member_type<I, E> is the type of the Ith member of E and reflect::get<I, E>(e) returns a reference to the Ith member of e.
    If E isn't an aggregate type reflect fails to compile.

Note that none of these cases require E to be an aggregate.

In summary I believe std::is_aggregate to be problematic because

  • it fails to catch types that support the "tuple-like" binding protocol which silently breaks this library (as explained under section 2.2)
  • it unnecessarily catches types that do not satisfy 1.1, 1.3, 1.4, 1.5.

3. Name related features

Currently, name related features (e.g. member_name(), has_member_name<T, Name>) require T to be default constructible because the library needs an instance of T to get its members names (at line 547). However, std::is_aggregate_v<T> does not imply that a type is default constructible. std::is_default_constructible can be used to check default constructability.

Neither std::is_aggregate<T> nor std::is_default_constructible is necessary though as there is a way to completely circumvent construction of an instance as shown by reflect-cpp. After a bit of digging I found out how they do it:

struct A {
	A() = delete; // not default constructible
	int fff;
	float bbb;
};
template <class T>
struct wrapper {
	const T value;
	static const wrapper<T> fake_object;
};
template <class T, std::size_t n>
struct fake_object_helper { };
template <class T> // partial specialization for class with 2 members, would need this for 0, 1, 2, 3, ...
struct fake_object_helper<T, 2> {
	template <int i> // get pointer to ith member
	static consteval auto get_field() {
		const auto& [f0, f1] = wrapper<T>::fake_object.value; // two members
		return [](const auto&... _refs) { return reflect::detail::nth_pack_element<i>(&_refs...); }(f0, f1);
	}
};

This code can be used like this reflect::detail::function_name<fake_object_helper<A, 2>::template get_field<0>()>() to create a string containing the name of the first member of A.

Even if you decide to stick to std::is_aggregate this trick is useful because it would allow the name related features to work on types which are aggregates but don't have a default constructor. The type and value related features already support those types.

4. Replacing std::is_aggregate

Well, unfortunatelly there is no suitable replacement for std::is_aggregate. AFAIK it is currently not possible to create a portable constraint which requires a type to support structured bindings. The only solution would be for the relevant functions/typedefs to not be constrained. I am aware that not using constraints isn't a great option as it will lead to compilation errors being triggered inside the library instead of in user code but in my opinion the advantage (increased applicability) outweighs the disatvantage (worse error messages).

It is at least worth a thought. And, as mentioned before, the trickery described in 3 can be implemented either way.

The bug described in 2.2 should probably get fixed as well. This can be achieved by requiring T to not support the "tuple-like" binding protocol and therefor excluding "tuple-like" types:

template<typename T>
concept doesnt_support_tuple_like_binding_protocoll = (! requires { std::tuple_size<T>::value; });
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

No branches or pull requests

1 participant