Skip to content
This repository was archived by the owner on Mar 21, 2024. It is now read-only.

Conversation

@ccecka
Copy link

@ccecka ccecka commented Apr 7, 2015

Implicit conversions are not considered during template argument deduction. Currently, stream output of a device_reference<A> where A is non-template (e.g. int, float, double, mystruct) functions as expected, but fails when A is a template (e.g. thrust::complex<float>, mytemplate<int>).

Example:

#include <thrust/device_vector.h>
#include <thrust/complex.h>
#include <iostream>

int main() {
  thrust::device_vector<double> a(5);
  std::cout << a[2] << std::endl;   // Outputs '0' as expected

  thrust::device_vector<thrust::complex<double> > b(5);
  std::cout << b[2] << std::endl;   // XXX: Fails to compile despite operator<< being defined for thrust::complex<T>
}

This patch simply uses static_cast to force the conversion and mirrors operator<< defined for device_ptr<T>.

@jaredhoberock
Copy link
Contributor

Thanks Cris. I think adding this functionality is OK, but a couple questions:

  1. Why an overload for basic_ostream and not just ostream?
  2. Instead of device_reference, this function should probably be overloaded for thrust::reference. device_reference will pick it up as a descendant of reference.
  3. is operator>> also required?

@ccecka
Copy link
Author

ccecka commented Apr 7, 2015

  1. ostream = basic_ostream<char, std::char_traits<char>> and wostream = basic_ostream<wchar_t, std::char_traits<wchar_t>>. User-defined char_traits are rumored to exist...
  2. I agree. The same should be done for device_ptr => thrust::pointer.
  3. Hadn't considered it. At the moment, op>> doesn't work for any thrust::device_reference<A> because operator A&() can't be implemented. I suppose an impl would be of the form
template <typename C, typename T, typename X>
std::basic_istream<C,T>& operator>>(std::basic_istream<C,T>& s, thrust::device_reference<X>& x) {
  typename thrust::device_reference<X>::value_type t;
  s >> t;
  x = t;
  return s;
}

I guess it's nice for symmetry with std::vector.

I'll make these changes tomorrow.

@jaredhoberock
Copy link
Contributor

Thanks. If operator>> doesn't already work with device_reference, let's not try to make it work right now. Also, same for pointer.

@ccecka
Copy link
Author

ccecka commented Apr 7, 2015

Stream output operators for thrust::pointer and thrust::reference with Doxygen stubs for thrust::device_ptr and thrust::device_reference.

thrust/complex.h Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't eliminate the newlines at the end of these files -- on some platforms, it causes a warning if the last line is not empty.

@jaredhoberock
Copy link
Contributor

Thanks Cris, I added some notes inline on your commit.

@ccecka
Copy link
Author

ccecka commented Apr 7, 2015

Clean diff.

@jaredhoberock
Copy link
Contributor

Thanks Cris, it looks good to me! I will test & merge your fixes tomorrow.

@jaredhoberock
Copy link
Contributor

The tests pass, so I can merge this change.

However, these fixes need to go into branch 1.8.2. Unfortunately, I can't change the target of this pull request -- can you target thrust:1.8.2 instead of thrust:master?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants