alpaka-group / alpaka

Abstraction Library for Parallel Kernel Acceleration :llama:
https://alpaka.readthedocs.io
Mozilla Public License 2.0
337 stars 69 forks source link

Allow non-integral types in Vec generator constructor #2236

Closed sliwowitz closed 3 months ago

sliwowitz commented 5 months ago

The explicit Vec(F&& generator, std::integer_sequence<TVal, Is...>) accepted a parameter pack of TVal which is defined to be the inner vector value and can possible be non-integral, which makes it invalid in pre-C++20 codebases. We've restricted the parameter to an integral type, thus disallowing this construction method for non-integral inner types.

mehmetyusufoglu commented 5 months ago

Is it possible to add a test for it?

sliwowitz commented 5 months ago

I suppose so. I've looked into the current Vec test, and it seems we're only testing integral types. Was this class ever supposed to handle floating point values?

sliwowitz commented 5 months ago

Seems @bernhardmgruber's idea works fine too. Does anyone have tips on what kind of additional tests to write for this? Maybe extend the current tests so that they'd consturct Vecs of additional types beside size_t, and add a test for the generator constructor (I can't see one in there)

psychocoderHPC commented 5 months ago

Seems @bernhardmgruber's idea works fine too. Does anyone have tips on what kind of additional tests to write for this? Maybe extend the current tests so that they'd consturct Vecs of additional types beside size_t, and add a test for the generator constructor (I can't see one in there)

Yes sounds good. We should have a test where a Vec<float,...> is created and for generators.

bernhardmgruber commented 5 months ago

Sure, please add tests for the generator constructor!

bernhardmgruber commented 4 months ago

@sliwowitz Could you please add a test so I can proceed merging this PR? Thanks!

psychocoderHPC commented 4 months ago

@sliwowitz ping

sliwowitz commented 4 months ago

Just a side note, Bernhard's solution actually allows non-integral types in the Vec generator constructor, so I renamed the PR to reflect that..