-
Notifications
You must be signed in to change notification settings - Fork 27
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
C++: use references for non-trivial types in accessors #1607
C++: use references for non-trivial types in accessors #1607
Conversation
{{resolveName typeRef}} {{resolveName this "" "getter"}}( ) const { return {{resolveName}}; }{{!! | ||
{{#ifPredicate typeRef "needsRefSuffix"}}const {{resolveName typeRef}}&{{!! | ||
}} {{resolveName this "" "getter"}}( ) const & { return {{resolveName}}; } | ||
{{resolveName typeRef}} {{resolveName this "" "getter"}}( ) const && { return {{resolveName}}; }{{/ifPredicate}}{{!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{{resolveName typeRef}} {{resolveName this "" "getter"}}( ) const && { return {{resolveName}}; }{{/ifPredicate}}{{!! | |
const {{resolveName typeRef}}&&{{resolveName this "" "getter"}}( ) const && { return std::move({{resolveName}}); }{{/ifPredicate}}{{!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning by-value is intentional here.
The proposed patch of return type may lead to dangling reference -- please see the toy-example that reproduces the possible problem (via usage of auto&&
):
https://godbolt.org/z/eMxMG3zEW
I would be tempted to apply std::move()
on a member field, but return by-value to avoid dangling references and at the same time avoid copying (example). But it needs to be done on a non-const member function overload.
Because, if the const is applied, then std::move()
will result in const RefType &&
, which will anyway call copy-constructor instead of move constructor (example here). In case of some types it may even cause compilation error, because some types from the standard library explicitly delete this overload (e.g. std::thread).
However, providing only &&
overload without providing && const
overload may lead to problems. Please see the following example.
So, as far as I understand, to avoid copying, but also prevent dangling references, three overloads of getter need to be provided:
// Called for all l-values.
const GType & get_g() const & {
return g;
}
// If this overload is not present, then a function returning 'const GType'
// will use the overload for l-value which causes dangling ref.
GType get_g() const && {
return g;
}
// Called on non-const rvalue.
GType get_g() && {
return std::move(g);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the problem by introducing 3 overloads of getter for non-trivial types.
Moreover, for such types 2 overload of setters have been added -- const lvalue ref
and rvalue ref
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code was updated according to the offline discussion.
This change is used to extend smoke tests for structures that use accessors C++ attribute. The goal of this change is to show the invalid behavior of C++ generator. The setters and getters use by-value returns and parameters even for non-trivial types. In the next patch the generator will be adjusted. The smoke test will be used to show the new behavior. Signed-off-by: Patryk Wrobel <[email protected]>
f914c76
to
13c0361
Compare
Note for reviewers -- the change is split into 3 commits:
|
The generated accessors used by-value return and parameter types even for non-trivial types. This change is intended to improve the situation by introducing usage of reference for the mentioned non-trivial types. This change: - introduces usage of const l-references for non-trivial types in getters defined via accessors attribute for structures when it is called on l-value - introduces usage of r-references for non-trivial types in getters defined via accessors attribute for structures when it is called on r-value - introduces usage of references for non-trivial types in setters (two overloads are provided -- const lvalue ref and rvalue ref) - strips constness and references from type used to select a hash in 'StructHashImpl.mustache' because std::hash<T> does not accept references/constness for T - updates output of smoke tests to show, that since this commit the generated accessors use const references for non-trivial types Signed-off-by: Patryk Wrobel <[email protected]>
This change implements a functional test for C++ accessors. It verifies the returned type of getters depending on the object type used to invoke the getter. The test contains also the usual runtime checks of returned values. Signed-off-by: Patryk Wrobel <[email protected]>
13c0361
to
e99a0f4
Compare
TEST( AccessorsOfMutableStruct, GettingFieldsOnRvalue ) | ||
{ | ||
// When using getter on rvalue. | ||
auto int_field = get_struct().get_trivial_int_field(); | ||
auto double_field = get_struct().get_trivial_double_field(); | ||
auto string_field = get_struct().get_nontrivial_string_field(); | ||
auto list_field = get_struct().get_nontrivial_list_field(); | ||
|
||
// Then access can be made. | ||
EXPECT_EQ(TrivialInt{32}, int_field); | ||
EXPECT_DOUBLE_EQ(TrivialDouble{27.5}, double_field); | ||
EXPECT_EQ(NontrivialString{"Some string"}, string_field); | ||
|
||
const NontrivialList expected_list{"S1", "S2", "S3"}; | ||
EXPECT_EQ(expected_list, list_field); | ||
} | ||
|
||
TEST( AccessorsOfMutableStruct, GettingFieldsOnConstRvalue ) | ||
{ | ||
// When using getters on const rvalue. | ||
auto int_field = get_const_struct().get_trivial_int_field(); | ||
auto double_field = get_const_struct().get_trivial_double_field(); | ||
auto string_field = get_const_struct().get_nontrivial_string_field(); | ||
auto list_field = get_const_struct().get_nontrivial_list_field(); | ||
|
||
// Then access can be made. | ||
EXPECT_EQ(TrivialInt{64}, int_field); | ||
EXPECT_DOUBLE_EQ(TrivialDouble{52.3}, double_field); | ||
EXPECT_EQ(NontrivialString{"Another string"}, string_field); | ||
|
||
const NontrivialList expected_list{"A1", "A2", "A3"}; | ||
EXPECT_EQ(expected_list, list_field); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: if these test cases used auto&&
, then we would have dangling references.
The generated accessors used by-value return and parameter
types even for non-trivial types. This change is intended
to improve the situation by introducing usage of reference
for the mentioned non-trivial types.
This merge request:
in getters defined via accessors attribute for structures
when it is called on l-value
in getters defined via accessors attribute for structures
when it is called on r-value
(two overloads are provided -- const lvalue ref and rvalue ref)
in 'StructHashImpl.mustache' because std::hash does not
accept references/constness for T
the generated accessors use const references for non-trivial
types
Note: the following changes extend smoke tests to show that
accessors defined for non-trivial types use references,
whether accessors defined for trivial types use values.