oneDPL icon indicating copy to clipboard operation
oneDPL copied to clipboard

Adjust transform_iterator constructors for default constructability checks

Open danhoeflinger opened this issue 1 year ago • 2 comments

The transform_iterator constructor has the potential to cause problems for checks of default constructability (and different results for different compilers clang vs gcc).

This PR makes clear to the compiler exactly when the transform_iterator is default constructible, where other simpler versions with default arguments or without the enable_if may result in build errors or the wrong value being returned when std::is_default_constructible is called on the transform_iterator.

This PR changes transform_iterator in that it only enables the constructors which are valid in the instantiation by separating out the constructors which rely upon default constructors of the template types, and only enabling them under the correct circumstances of default constructability of template arguments.

This is a better fix for the issue which motivated #1432. However, I think both are changes for the better.


Update: The PR has grown to include:

  1. Changes to constructors of transform_iterator to enable default constructability checks
  2. removal of const from the unary functor within transform_iterator
  3. new utility: __is_const_callable_with_arg_v
  4. testing changes to match changes in code and test fixed functionality
  5. documentation changes to be more explicit about eligible functors for use with transform_iterator

danhoeflinger avatar Mar 13 '24 01:03 danhoeflinger

So, this change has an consequence...

LegacyForwardIterator and therefore LegacyBidirectionalIterator and LegacyRandomAccessIterator requires default constructible.

The way transform_iterator was written, it was returning true for is_default_constructible even when it was not the case (lambdas are not default constructible in C++17). Now it accurately returns false when the unary functor is not default constructible.

Practically, we may not have an issue here, other with our tests which are static_assert default constructible. We need to consider the consequences of this change though. Within transform_iterator, we carry forward the iterator category from the source iterator, regardless of the functor.

danhoeflinger avatar Mar 14 '24 14:03 danhoeflinger

The clang format suggestion is a bug in clang format where it mistakes a binary AND for a rvalue reference. It is intentionally ignored.

danhoeflinger avatar Mar 15 '24 12:03 danhoeflinger

Thanks all for the reviews! Very interesting investigation I think, and I like where it ended up.

danhoeflinger avatar Apr 16 '24 14:04 danhoeflinger