Compilation failure in uninitialized_{fill,copy}{_n} for certain classes
- Dominant language
- C++
- Stars
- 2.5k
- Forks
- 487
- Avg merge
- 2d 7h
- Merged PRs (30d)
- 296
Description
Both `uninitialized_fill` and `uninitialized_copy` (as well as their `*_n` versions) use the copy constructor to copy element(s) to the provided range of memory. While the basic implementation is rather straightforward and compliant, thrust seems to use an optimization for certain types/classes. In particular, it tries to detect whether the underlying type `T` is trivially copy constructible and then calls the regular `fill` and `copy` algorithms (same for `*_n` versions). However, the latter functions use the copy assignment operator to copy the element(s) which may lead to compilation failures in case a class has a (trivial) copy constructor but no copy assignment operator. Consider this example:
```
using T = thrust::pair;
T x(10, 2.0f);
thrust::device_vector d_v(1000);
thrust::uninitialized_fill(d_v.begin(), d_v.end(), x); // May fail to compile
```
To be more precise, the optimization uses the type trait `thrust::detail::has_trivial_copy_constructor` which is incorrect in above case as `T` obviously has a trivial copy constructor but no copy assignment operator, so the executed `fill` operation will fail to compile. Even worse, this error only triggers if the OpenMP (and maybe also TBB) backend is used because the implementation is compiler-dependent, i.e. contains additional checks for GCC, Clang, and MSVC. For the (default) CUDA backend and the nvcc compiler, the error does not occur.
At first glance, the solution would be to use `thrust::detail::has_trivial_assign` which is also provided. However, this also does not work for the above example using the OpenMP (and TBB) backend (CUDA backend still not affected):
```
using T = thrust::pair;
// true, does not trigger -> false positive
static_assert(thrust::detail::has_trivial_assign()(), "C++98 thrust implementation");
// false, triggers as expected
static_assert(std::is_trivially_copy_assignable::value, "C++11 standard implementation");
```
The reason for this lies in the fact that it uses the compiler-specific type trait `__has_trivial_assign` which is used only for GCC, Clang and MSVC and has a too permissive behavior causing false positives (see e.g. https://gcc.gnu.org/onlinedocs/gcc-4.7.4/gcc/Type-Traits.html).
Therefore, there as several possibilities to fix this issue which I would like to discuss first before going ahead and providing a fix:
1. Remove the optimization. This is the easiest and most reliable solution to make the implementation compliant for all use cases and also works for C++98. The downside of this solution is that there might be performance regressions. I have not measured the performance difference between the uninitialized and the regular algorithms. Note that `thrust::detail::has_trivial_assign` still needs to be fixed to avoid false positives (see 2.).
2. Use `thrust::detail::has_trivial_assign` and remove the unreliable and compiler-dependent `__has_trivial_assign` expression (which makes it equal to `thrust::detail::is_pod` and the version used for the CUDA backend). This keeps the optimization at least for POD types and stills works as expected. As for 1. the performance may degrade.
3. Use 2. and conditionally define `thrust::detail::has_trivial_assign` to be equivalent to (i.e. make use of) `std::is_trivially_copy_assignable` if C++11 or a higher standard is detected. This is probably the fastest solution although there might be still performance regressions (however, in C++98 only).
Solution 2 is probably a good compromise if the optimization should be kept. The last solution also takes advantage of the more recent C++11 standard (which most users probably already use).
As a final remark: Although this example may look like an uninteresting/artificial corner case, there are real-world uses cases for this. An example is the stdgpu library (https://github.com/stotko/stdgpu) and its `stdgpu::unordered_map` container which uses `thrust::pair`. To circumvent this issue, it currently implements a workaround which is equivalent to solution 1 (see https://github.com/stotko/stdgpu/pull/26).
Contributor guide
Assessment
This issue has not been assessed yet.