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

fix(lifetime): extending the lifetime of temporary objects #160

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from

Conversation

amitsingh19975
Copy link
Collaborator

The current implementation does not take into account the prvalue that
creates an issue while storing it. The prvalue is destroyed after the
end of its scope, and if you try to access it, you are entering into
the undefined land of C++.

To solve this issue, we employ the trait std::is_lvalue_reference to
determine if need to extend the lifetime or not. If an extension is needed,
we store the value with the help of universal forwarding. Otherwise, we
extend the lifetime using const &.

Resolves #125, resolves #118

Fix for issue #125.

The current implementation does not take into account the prvalue that
creates an issue while storing it. The prvalue is destroyed after the
end of its scope, and if you try access it, you are entering into
undefined land of C++.

To solve this issue, we employ the trait `std::is_lvalue_reference` to
determine if need to extend the lifetime or not. If extension is needed,
we store the value with the help of universal forwarding. Otherwise, we
extend lifetime using `const &`.
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Reports by clang tidy

@boostorg boostorg deleted a comment from github-actions bot Feb 9, 2022
@boostorg boostorg deleted a comment from github-actions bot Feb 9, 2022
@boostorg boostorg deleted a comment from github-actions bot Feb 9, 2022
@boostorg boostorg deleted a comment from github-actions bot Feb 9, 2022
@boostorg boostorg deleted a comment from github-actions bot Feb 9, 2022
@boostorg boostorg deleted a comment from github-actions bot Feb 9, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Reports by clang tidy

examples/tensor/simple_expressions.cpp Show resolved Hide resolved
examples/tensor/simple_expressions.cpp Show resolved Hide resolved
examples/tensor/simple_expressions.cpp Outdated Show resolved Hide resolved
examples/tensor/simple_expressions.cpp Show resolved Hide resolved
examples/tensor/simple_expressions.cpp Show resolved Hide resolved
examples/tensor/simple_expressions.cpp Show resolved Hide resolved
Decoupled each `constexpr if` conditions into function using requires
clause, which cleaned-up the compare function. Also, added the check for
static extents that removes the runtime check.
@github-actions
Copy link

This Pull request Passed all of clang-tidy tests. 👍

Since the patch is fixing the lifetime of the tensor, we cannot combine a prvalue with the old ublas expression. Therefore, we have to bind the prvalue with the name to increase the lifetime.
This function casts any `tensor_expression` to its child class, and it
also handles recursive casting to get the real expression that is stored
inside the layers of `tensor_expression`.
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Reports by clang tidy

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Reports by clang tidy

include/boost/numeric/ublas/tensor/expression.hpp Outdated Show resolved Hide resolved
include/boost/numeric/ublas/tensor/expression.hpp Outdated Show resolved Hide resolved
include/boost/numeric/ublas/tensor/expression.hpp Outdated Show resolved Hide resolved
@boostorg boostorg deleted a comment from github-actions bot Feb 13, 2022
@boostorg boostorg deleted a comment from github-actions bot Feb 13, 2022
Comment on lines 208 to 210
constexpr bool operator==( typename T::value_type lhs, boost::numeric::ublas::detail::tensor_expression<T,D> const& rhs) noexcept{
return boost::numeric::ublas::detail::compare( rhs, [lhs](auto const& r){ return lhs == r; } );
}
Copy link
Collaborator Author

@amitsingh19975 amitsingh19975 Feb 13, 2022

Choose a reason for hiding this comment

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

Should we use the std::move, or are we only expecting the tensor to have standard C++ arithmetic types and don't need to care about other types, even user-defined types?

Suggested change
constexpr bool operator==( typename T::value_type lhs, boost::numeric::ublas::detail::tensor_expression<T,D> const& rhs) noexcept{
return boost::numeric::ublas::detail::compare( rhs, [lhs](auto const& r){ return lhs == r; } );
}
constexpr bool operator==( typename T::value_type lhs, boost::numeric::ublas::detail::tensor_expression<T,D> const& rhs) noexcept{
return boost::numeric::ublas::detail::compare( rhs, [lhs = std::move(lhs)](auto const& r){ return lhs == r; } );
}

Previously, we would create a new tensor from the expression template
every time there is a comparision. Also, fixed the spelling to
`cast_tensor_expression` from `cast_tensor_exression`.
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Reports by clang tidy

include/boost/numeric/ublas/tensor/expression.hpp Outdated Show resolved Hide resolved
include/boost/numeric/ublas/tensor/expression.hpp Outdated Show resolved Hide resolved
include/boost/numeric/ublas/tensor/expression.hpp Outdated Show resolved Hide resolved
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Reports by clang tidy

The standard does not define the complex class other than float, double,
and long double. It leaves the implementation on the hands of vendors,
and it could lead to the portability issue or undefined behaviour, which
is not warranted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant