search

martinmoene / span-lite

span lite - A C++20-like span for C++98, C++11 and later in a single-file header-only library
Boost Software License 1.0
499 stars 43 forks source link

Question regarding the design of the extension functions (subspan etc.) #65

Closed vvish closed 3 years ago

vvish commented 3 years ago

Hello,

I have a question regarding the design of the free functions:subspan, first and last: As they do not explicitly accept span<T> are they intended to be used with any type that can be accepted by make_span function? If it is the case then the implementation seems to have an issue: as ADL is not finding them with non-span-specialization parameters the call to this functions should be qualified (currently they are not available in the nonstd namespace) and make_span in the trailing return type expression should be visible for non-ADL. Unfortunately the unit-tests cover only the cases where the span specialization is passed to the functions (works because of ADL), so it is not clear if the use-case was considered, although I personally find it to be useful (would be glad to contribute here).

If the functions are designed to be used with span-s only as wrappers for methods to avoid .template then maybe they could accept span<T> ?

Could you please clarify the use cases and rational behind the design?

Thank you.

martinmoene commented 3 years ago

Hi @vvish

Thanks for bringing this up.

I've trouble to trace back where the idea for non-member first() etc. comes from. Given these functions are not available from namespace nonstd carries the suggestion that the idea/implementation isn't 'fully baked'.

Think the implementation is chosen to reduce the number of overloads needed by letting make_span()do most of the work. Wondering if the wide contract of first() etc. can lead to problems.

The changes in your fork seem good to me (after a quick-ish look).

Whether it would be better & enough to have first() etc. accept span<T> as argument, I'd appreciate to be educated on :)

vvish commented 3 years ago

Thank you @martinmoene for the quick reply.

The documentation https://github.com/martinmoene/span-lite/blob/master/README.md#first-last-and-subspan suggests that they were introduced "to avoid having to use the dot template syntax when the span is a dependent type". It looks like if it is the only intended usage then they could receive already constructed span<T> and simply delegate call to the member (this needs to be validated).

If they also act as 'factory' functions constructing sub-spans from span-compatible types (instead of chaining, like make_span().first() ), then they require wide contract and changes from my patch seem to be relevant.

I think in both cases, this functions should be available in nonstd namespace.

martinmoene commented 3 years ago

Hi @vvish

I think merging the changes you prepared seems a good start.

Then look if first() etc. can be made to take span<T> as argument or if they should remain factory functions.

vvish commented 3 years ago

Hi @martinmoene , That was quick :). Thank you.

And thank you very much for your lib-s. They are of great aid.

martinmoene commented 3 years ago

Thank you, nice to hear :) You're welcome.

Perhaps continue with a new issue like 'Change free functions first(), last(), subspan() to take a span?' to investigate this aspect?