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

Bug/GitHub/wconversion-1478 Check in #1623

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

matt-stack
Copy link
Contributor

In reference to NVIDIA/cccl#779

Creating PR to get vis on the last remaining tricky type conversions.

The two pain points are

  1. the conversions required for thrust/system/detail/sequential/scan.h (and similarly for thrust/system/detail/sequential/scan_by_key.h)
  2. the 4 spots I detail below

Attaching the list of remaining warnings here:
conversionWarning47.txt

There are four spots where I had a pragma ignore "-Wconversions" because those spots were called so often they would blow up the log, and tricky to solve so I moved on. (Thus they are not present in the log above). I made attempts to create general conversions for them, but was unsuccessfully at the time.

These 4 spots are:
./detail/function.h around line 97
./detail/internal_functional.h around line 332
./detail/tuple.inl (has two spots) around line 479 and 367

I am compiling a log including these errors now, will attach after the fact.

I do not believe I made any changes to the tests themselves, only the source of the type conversions. There are some tests that deliberately test a rotating array of types against each other.

I believe a trained eye could identify the solutions to the trickier ones that have eluded me.

@@ -54,6 +54,8 @@ __host__ __device__

// Use the input iterator's value type per https://wg21.link/P0571
using ValueType = typename thrust::iterator_value<InputIterator>::type;
// Use for explicit type conversion
using OutputType = typename thrust::iterator_value<OutputIterator>::type;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was my thinking, that the output type would make sense, but when I do include this I get compile time errors

Copy link
Collaborator

Choose a reason for hiding this comment

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

Output iterators are not required to have a value type.

Copy link
Contributor Author

@matt-stack matt-stack Apr 6, 2022

Choose a reason for hiding this comment

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

@ericniebler Thanks, for those that dont have it is value type set to null?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We've been using this pattern in similar situations: https://github.com/NVIDIA/cub/blob/769e149dde423101dec8e618511e4fac8ba7bfe7/cub/util_type.cuh#L82-L94

We can't directly use that cub trait here since this file isn't in the CUDA backend, but we could add a similar one in thrust/detail/type_traits.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tips, I will look into implementing that. There may still be times where when it has to pick the Fallback type (which I am sensing would be ValueType to not break anything), there may still be warning.

@@ -312,6 +312,7 @@ template <class AdaptableUnaryFunction, class Iterator, class Reference = use_de
// convert to their value type before calling m_f. Note that this
// disallows non-constant operations through m_f.
typename thrust::iterator_value<Iterator>::type const& x = *this->base();
// The issue is that x needs to be the type specified in m_f, but that is unknown
return m_f(x);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried

return static_cast<typename super_t::reference>(m_f(x));

To this one, but I believe it also caused some compile time errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looked at this again, and not sure if there is a way (without adding extra args) to inherit the type expected by the unary operator m_f, might have to be ignored from -Wconversion?

@alliepiper alliepiper added type: enhancement New feature or request. P1: should have Necessary, but not critical. labels Mar 7, 2022
@alliepiper alliepiper added this to Inbox in PR Tracking via automation Mar 7, 2022
@alliepiper alliepiper added this to the 1.17.0 milestone Mar 7, 2022
@alliepiper alliepiper moved this from Inbox to Need Review in PR Tracking Mar 7, 2022
@alliepiper alliepiper removed this from the 1.17.0 milestone Apr 25, 2022
@alliepiper alliepiper added this to the Backlog milestone Apr 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P1: should have Necessary, but not critical. type: enhancement New feature or request.
Projects
PR Tracking
Need Review
Development

Successfully merging this pull request may close these issues.

None yet

3 participants