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

Wrong assignment between Array<TinyVector> and TinyVector expression #108

Open
AnderOne opened this issue Mar 29, 2019 · 12 comments
Open

Wrong assignment between Array<TinyVector> and TinyVector expression #108

AnderOne opened this issue Mar 29, 2019 · 12 comments

Comments

@AnderOne
Copy link

There is a problem with to unwrap of expression templates when we use multicomponent Array on the left side and a vector expression on the right.

#include <blitz/array.h>
#include <iostream>

int main() {

	blitz::Array<blitz::TinyVector<double, 2>, 1> DAT(5);
	blitz::TinyVector<double, 2> TMP(3, 7);
#if false
	//Here we construct TinyVector from expression and fill Array. It's right!
	DAT = blitz::TinyVector<double, 2> (TMP * 2 - 1);
#else
	//This code leads to incorrect behavior!
	DAT = TMP * 2 - 1;
#endif
	std::cout << DAT << std::endl;

	return 0;
}

If N-dimensional array is used then it leads to compilation error:

#include <blitz/array.h>
#include <iostream>

int main() {

	blitz::Array<blitz::TinyVector<double, 2>, 2> DAT(5, 4);
	blitz::TinyVector<double, 2> TMP(3, 7);
#if false
	//Here we construct TinyVector from expression and fill Array. It's right!
	DAT = blitz::TinyVector<double, 2> (TMP * 2 - 1);
#else
	//This code leads to compilation error!
	/*./blitz/blitz/array/expr.h:194:81:
	error: no match for call to
	‘(const T_expr {aka const blitz::FastTV2Iterator<double, 2>})
	(const blitz::TinyVector<int, 2>&)’
	T_result operator()(const TinyVector<int, N_rank>& i) const
	{ return iter_(i); }
	*/
	DAT = TMP * 2 - 1;
#endif
	std::cout << DAT << std::endl;

	return 0;
}
@citibeth
Copy link
Contributor

citibeth commented Mar 29, 2019 via email

@AnderOne
Copy link
Author

I could suggest make TinyVector as scalar type without expression template support.
Then all arithmetic operators between TinyVectors will return TinyVector as value.
Also assignment operator can get appropriate specialization.
It's simplest way to solve this problem.

@lutorm
Copy link
Collaborator

lutorm commented Mar 30, 2019 via email

@citibeth
Copy link
Contributor

citibeth commented Apr 1, 2019 via email

@AnderOne
Copy link
Author

AnderOne commented Apr 5, 2019

It seems that I have solved the problem.
But for this purpose I had to use the means of C++11 (from <type_traits>):
https://github.com/AnderOne/blitz/commits/test_for_expr

I'm not sure that my code works correctly in all possible cases,
but it passes all existing tests.

If you decide that it's useful, I could send a pull request.

@citibeth
Copy link
Contributor

citibeth commented Apr 5, 2019 via email

@AnderOne
Copy link
Author

AnderOne commented Apr 7, 2019

PR is here: #109

@lutorm
Copy link
Collaborator

lutorm commented Apr 7, 2019 via email

@citibeth
Copy link
Contributor

citibeth commented Apr 7, 2019 via email

@lutorm
Copy link
Collaborator

lutorm commented Apr 7, 2019 via email

@AnderOne
Copy link
Author

AnderOne commented Apr 8, 2019

My patch doesn't break ET logic. It just make checking for a type conversion between
right side expression and Array::T_numtype in a compile time.
However, this feature requires C++11.

I known that migrate to a modern C++ repeatedly discussed.
But in this case, it really
fixes a bug.

@citibeth
Copy link
Contributor

citibeth commented Apr 8, 2019 via email

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

3 participants