Open rnikander opened 1 year ago
detail::diffmax_t
's conversion operator is explicit
, and the implementation of concat
fails to account for that.
detail::diffmax_t
's conversion operator isexplicit
, and the implementation ofconcat
fails to account for that.
Why is it explicit
? It's easy enough to go through and change https://github.com/ericniebler/range-v3/blob/689b4f3da769fb21dd7acf62550a038242d832e5/include/range/v3/view/concat.hpp#L176 to be
ranges::advance(it.get(), static_cast<iter_difference_t<I>>(n));
in the dozen or so odd places where this needs to happen, but ...?
That's the rule we ended up with in the standard, courtesy of P1522 but I'm not ... sure why.
Because potentially-narrowing conversions in C++ should not happen implicitly.
Which was also changed by https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2393r1.html to be conditionally explicit.
The proposed views::concat
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2542r2.html seems to use a different strategy to advance, even though both this implementation and the proposed one are non-borrowed.
The proposed
views::concat
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2542r2.html seems to use a different strategy to advance, even though both this implementation and the proposed one are non-borrowed.
@huixie90 Every use of +
and -
with an iterator and an integral type in that wording needs to convert the integral to the iterator's difference type. We don't require random access iterators to accept arbitrary integer-like types, they're only required to accept their difference type. (Queue war story about x + 1
in the implementation of vector
breaking a user program.)
Looking at range-v3, the integer type passed to advance
is the common type of the difference types of the ranges in concat_view
. The conversion to the actual ith iterator type's difference type should be in range.
Because potentially-narrowing conversions in C++ should not happen implicitly.
So should we use narrow_cast
on all such conversions? https://github.com/ericniebler/range-v3/blob/689b4f3da769fb21dd7acf62550a038242d832e5/include/range/v3/view/span.hpp#L51-L60
The proposed
views::concat
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2542r2.html seems to use a different strategy to advance, even though both this implementation and the proposed one are non-borrowed.@huixie90 Every use of
+
and-
with an iterator and an integral type in that wording needs to convert the integral to the iterator's difference type. We don't require random access iterators to accept arbitrary integer-like types, they're only required to accept their difference type. (Queue war story aboutx + 1
in the implementation ofvector
breaking a user program.)
thanks for spotting that.
If such conversion is explicit, there must be a reason (e.g narrowing). Instead of making concat
to do the conversion (potentially narrowing) , would it be more appropriate to disallow such inputs all together by adding more constraints to concat?
Irrespective of the range of values in the the difference type, is the size of the range. The implementation is calling advance(it, n)
, so n
must already be such that conceptually [it, it + n)
is a valid range. It just so happens that it's using a bad type for n
. Otherwise, it's a bug in the implementation.
So should we use
narrow_cast
on all such conversions?
I think this would be a good idea if narrow_cast
were extended to accept integer-like types.
If such conversion is explicit, there must be a reason (e.g narrowing). Instead of making concat to do the conversion (potentially narrowing) , would it be more appropriate to disallow such inputs all together by adding more constraints to concat?
That would effectively mean that concat
only works when all constituent ranges have the same difference type, which is IMO throwing out the baby with the bathwater. Like Johel says, we always know that the actual values used are in the representable range, otherwise e.g. get<N>(it_) += steps;
would have UB regardless of the type of steps
.
This error message has two problems:
I get that template C++ error messages are difficult but this seems like it might be far enough to be considered a bug.
Perhaps it's specific to the compiler. I'm using Clang 15.0.1
The error message is attached.
error.txt