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

Feature add variant_cast #129

Merged
merged 8 commits into from
Mar 8, 2018
Merged

Conversation

acki-m
Copy link
Contributor

@acki-m acki-m commented Mar 5, 2018

Added global "variant_cast" method

Added 4 overloads of the method "variant_cast"
In order to move values out, to return values by reference
or to return a pointer to the underlying values, when the type does match:

template
T variant_cast(variant&& operand);

template
T variant_cast(variant& operand);

template
T variant_cast(const variant& operand);

template
const T* variant_cast(const variant* operand) RTTR_NOEXCEPT;

template
T* variant_cast(variant* operand) RTTR_NOEXCEPT;

Additionally:

  • added non-const version of variant::get_value()
  • fixed copy-right year in root CMakeLists.txt file

Fixes #108

acki-m added 2 commits March 5, 2018 23:33
Added 4 overloads of the method "variant_cast"
In order to move values out, to return values by reference
or to return a pointer to the underlying values, when the type does match:

template<class T>
T variant_cast(variant&& operand);

template<class T>
T variant_cast(variant& operand);

template<class T>
T variant_cast(const variant& operand);

template<class T>
const T* variant_cast(const variant* operand) RTTR_NOEXCEPT;

template<class T>
T* variant_cast(variant* operand) RTTR_NOEXCEPT;

Additionally:
- added non-const version of variant::get_value()
- fixed copy-right year in root CMakeLists.txt file

Fixes rttrorg#108
@acki-m acki-m requested a review from gabyx March 5, 2018 22:51
@acki-m acki-m changed the title Feature add variant cast Feature add variant_cast Mar 5, 2018
@coveralls
Copy link

coveralls commented Mar 7, 2018

Coverage Status

Coverage increased (+0.03%) to 92.382% when pulling 6a9fb30 on acki-m:feature-add-variant-cast into 621cd1d on rttrorg:master.

@acki-m
Copy link
Contributor Author

acki-m commented Mar 7, 2018

@gabyx
What do you think?

{
variant var = 12;

auto value = variant_cast<int>(&var);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that correct: value is still bound to the var. meaning if var gets destroyed, value is dangling as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that is correct. Same as with variant::get_value<T>()

* \code{.cpp}
*
* variant var = std::string("hello world");
* std:string* a = variant_cast<std::string>(&var); // performs an internal type check and returns extracts the value by reference
Copy link
Contributor

Choose a reason for hiding this comment

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

remove extracts

* std:string* a = variant_cast<std::string>(&var); // performs an internal type check and returns extracts the value by reference
* int* b = variant_cast<int>(&var);
* std::cout << "a valid: " << a != nullptr << std::endl;
* std::cout << "b valid: " << b != nullptr << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably add "// b is nullptr"

{
const variant var = 12;

auto value = variant_cast<int>(&var);
Copy link
Contributor

Choose a reason for hiding this comment

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

so the const semantic in variant_cast<...> is neglected. meaning we dont write variant_cast<const int>.
Wouldnt that be better? I see the benefits of the current implementation. I would leave it like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gabyx
What would be better?
You cannot have a const-by-value inside a variant. However, you can have a const int*


REQUIRE(var.get_type() == type::get<int>());

auto value = variant_cast<int>(var);
Copy link
Contributor

Choose a reason for hiding this comment

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

;) -> auto has the semantic of always stripping references == &. So I think when you write auto value this is always an lvalue (not an lvalue reference).
So it seems that this test is obsolete :-), but better leave it included :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, but also this case can happen, thats why I added the test.
Its also in the section: SECTION("by value")

* \return A reference to the stored value.
*/
template<typename T>
T& get_value();
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that one. So such a functionality was not yet there. Its up to the user to call that with the right type.

@@ -652,7 +652,7 @@ struct RTTR_API variant_data_policy_empty
}
case variant_policy_operation::GET_VALUE:
{
arg.get_value<void*>() = nullptr;
arg.get_value<const void*>() = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

was that wrong before? and did you already had a get_value function before but not in the interface in variant (I suppose arg is a variant?)

Copy link
Contributor Author

@acki-m acki-m Mar 8, 2018

Choose a reason for hiding this comment

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

@acki-m acki-m merged commit 6e28c69 into rttrorg:master Mar 8, 2018
@acki-m acki-m deleted the feature-add-variant-cast branch March 8, 2018 20:31
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.

rttr::variant why only const getter?
3 participants