Add spec compliant histogram API while supporting legacy extension API

[danhoeflinger]

This PR adjust the API overloads for histogram to create an API which complies to the current PR for the specification, while continuing to support the existing API as an extension.

It removes an unnecessary and unused default parameter for ValueType which was incorrectly added in the original PR. The new spec compliant API implements what was the intended signature originally motivated from this discussion.

This change in overload / signature should intercept the normal usages of histogram which have matching types for the bound limits with the value type of the input iterators, and use the spec compliant signature.

For those using histogram that provide bound limits which do not match the value type of the input iterator, the legacy overload will still support this usage.

We should not stop supporting any existing working cases with this change, but we will add some additional supported invocations where the type of the boundaries do not match each other. If they are both implicitly convertible to the value type of the input iterator, they will be converted, and it should work (possibly with warnings).

We also remove the default for ValueType here for the legacy API without harm, as it can never be used in practice.

Example in godbolt to play with: Godbolt example

Updated godbolt: https://godbolt.org/z/Pev1Ps8nY

(Version 2 from email)

[timmiesmith]

LGTM.

[akukanov]

I do not see why this extra template parameter is needed. As far as I know, it is fine to have function template overloads with different numbers of template parameters.

[danhoeflinger]

In combination with the other overload, this unused defaulted template parameters covers the case where a user specified all template parameters explicitly, and the specified type matches the input iterator value type.

histogram<Policy, int*, std::size_t, uint32_t*, int>(policy, first, last, num_bins, bin_min, bin_max, histogram_first);

The other overload would exclude it because of the matching type.

[akukanov]

Since C++20, the standard has a clause in [algorithms.requirements] similar to this (taken from the current draft):

The well-formedness and behavior of a call to an algorithm with an explicitly-specified template argument list is unspecified, except where explicitly stated otherwise.
[Note 3: Consequently, an implementation can declare an algorithm with different template parameters than those presented. — end note]

I believe this was added due to range algorithms, which are usually implemented in a way that they cannot be called with explicit template arguments. The restriction however, as I read it, is not limited to range algorithms.

Since we are adding parallel range algorithms both to the library and the spec as well, we will need a similar clause, and maybe it makes sense to apply to all algorithms.

[danhoeflinger]

Very interesting, I wasn't aware of that.

[akukanov]

I wonder if keeping the old variant of the function available sort of defeats the purpose of adding the new one, as Dan wrote in a comment to the specification PR:

I think it is better for force users to provide bounds which are convertible to the value_type of the input sequence, and do the conversion once when accepting the arguments rather than each time in the comparison operator.

If this overload is kept under the condition of type not being the same as the iterator value type, it will be selected even for the cases where we would want to have the conversion done only once. If the condition is changed to that of type not being convertible, will the code even work for such types? Or should the condition be for types that are not implicitly convertible to the desired one, while still explicitly convertible?

[danhoeflinger]

I need to experiment here a little to see if there is a better check. I think if we check !std::is_convertible instead, I think it should still work.

[danhoeflinger]

Technically, I believe this works with !std::is_convertible_v in place of !istd::is_same_v in that all the cases which build and run before still build and run in the new version as well.

However, the unused and defaulted template parameter feels a little more hacky with this version. With the !std::is_same_v version, we know that the only time the spec compliant signature is picked, the types are the same, so doing this conversion is a no-op. Alternately, with !std::is_convertible_v, if someone explicitly lists out their parameter list, including a _ValueType which is convertible but not the same as the input iterator value type to match the old signature, we would still pick the new spec compliant overload and do the conversion.

I have an improved version in a sandbox godbolt which adds a (sigh) 4th overload, but manages to remove the hacky unused and defaulted template parameter from the spec compliant signature.

This version breaks up the extension overloads into two signatures:

1. type not convertible to input iterator value type
   This is basically what we have here, but checking !std::is_convertible_v

2. Explicit template list
   This makes the 4th template argument non-deducable, but has 5th and 6th template arguments to represent the types of the two boundary limit parameters

I'll get an commit in to update this PR with what it would look like tomorrow, its better, but I think I still prefer 1a or 1b.

[danhoeflinger]

I've updated the code to implement the improved logic for supporting the extension API cases, and added tests to check these overloads specifically.

[danhoeflinger]

After discussion with @rarutyun and @MikeDvorskiy, I think we may have better options here (specification / implementation). I will provide links for the alternatives tonight or tomorrow. I sent and email with some details which you can look at in the mean time.

[akukanov]

Should we claim the old APIs deprecated, so that their use causes warnings and encourages the change to the supported overload?

[akukanov]

It makes sense to mention that _ValueType is not deducible and so this overload can only be chosen if it's explicitly specified.

[danhoeflinger]

Yes, I should mention that specifically, I'll fix the comment.

[danhoeflinger]

Fixed.

[danhoeflinger]

Should we claim the old APIs deprecated, so that their use causes warnings and encourages the change to the supported overload?

I'm OK with that. Does that have any implication on the oneDPL version numbering, etc.?

[akukanov]

Does that have any implication on the oneDPL version numbering, etc.?

The version is not impacted, as the API is still there. But it makes sense to check what needs to be done to claim something deprecated. Tagging @timmiesmith.

[timmiesmith]

Does that have any implication on the oneDPL version numbering, etc.?

The version is not impacted, as the API is still there. But it makes sense to check what needs to be done to claim something deprecated. Tagging @timmiesmith.

Deprecations have to be announced 1 year before the API is removed, and it's encouraged to do it at the major oneAPI release. We need to add the warning macros to the APIs so the user does see the warnings at comepile time now, and then next year we can remove the APIs. 5c4840f is an example of the notice required.

[danhoeflinger]

Deprecations have to be announced 1 year before the API is removed, and it's encouraged to do it at the major oneAPI release. We need to add the warning macros to the APIs so the user does see the warnings at comepile time now, and then next year we can remove the APIs. 5c4840f is an example of the notice required.

OK, I can add deprecation warnings here to the extension APIs. Thanks @timmiesmith.

[danhoeflinger]

Deprecations have been added. Its added a clang-format suggestion which I very much disagree with.

[danhoeflinger]

We determined that we want to go with #1800 instead. Converting to draft and removing from milestone.

[danhoeflinger]

closing this in favor of #1800.