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
495 stars 40 forks source link

span_FEATURE_*_TO_STD does not do what is advertised #32

Open Flamefire opened 5 years ago

Flamefire commented 5 years ago

Example:

-Dspan_FEATURE_MAKE_SPAN_TO_STD=14 Define this to the highest C++ language version for which to provide creator functions nonstd::make_span(). Default is undefined.

  1. I do not understand why? It sounds like defining this to "14" leads to make_span available in 98, 11, 14 and not available in 17, 20, ... Why would one want this?
  2. It does not work like that: #define span_IN_STD( v ) ( (v) == 98 || (v) >= span_CPLUSPLUS_V )
    • When defined to 98 it will always be available
    • Otherwise see 1.

Is this intentional? If so, can this be documented?
Why not make it simply a ON/OFF define? The build system can then be used to provide those in a more transparent way. (e.g. CMake generator expressions)

martinmoene commented 5 years ago

Ad 1. span_FEATURE_MAKE_SPAN_TO_STD

std::span is a C++20 type and no std::make_span() will be provided as there's no need for these since C++17 thanks to class template argument deduction (guides).

span-lite provides a span type for C++98 and later and does provide make_span() to ease construction of spans. Being able to control the presence of make_span() provides the user with a migration path of their choice into C++17 and later.

The expected number should have been document as with -Dspan_FEATURE_WITH_CONTAINER_TO_STD, thanks!.

Ad 2. The desired order is (C++) 98, 03, 11, 14, 17, 20...

"It looked like a good idea at the time" and it's one way of doing it; an on/off toggle like span_FEATURE_MAKE_SPAN would be another.

martinmoene commented 5 years ago

Thanks, I'll look at the implementation of span_IN_STD( v ) at a later time.

Flamefire commented 5 years ago

My point is: If you use this now with make_span enabled and somehow (e.g. using CMake and a dependent library requires a newer C++ standard which is propagated to yours) then suddenly your code stops working as make_span is no longer provided. (This happened to me which is why I opened this)

So either provide it or don't provide it. I even run into a situation where 1 part was compiled with C++11 and one with C++14 and make_span got defined in one but not the other but one used the other and got broken. The CMake way (and IMO correct way) is to propagate requirements upwards. So if a dependent library requires make_span (in its interface) it defines the define and propagates this to consumers of this library. If the consumer uses a newer C++ standard this requirement gets effectively ignored.

So please provide at least additionally an enable flag.

Ad 2. The desired order is (C++) 98, 03, 11, 14, 17, 20...

Per your own docu: // C++ language version (represent 98 as 3):

Additionally: If you don't provide make_span then the current implementation can also not provide the subspan functions

martinmoene commented 5 years ago

Fix of span_IN_STD:

#define span_IN_STD( v )  ( ((v) == 98 ? 3 : (v)) >= span_CPLUSPLUS_V )
martinmoene commented 5 years ago

My point is: If you use this now with make_span enabled and somehow (e.g. using CMake and a dependent library requires a newer C++ standard which is propagated to yours) then suddenly your code stops working as make_span is no longer provided.

This accurately describes the use case I had in mind: Set the C++ standard at which one wants to be reminded to replace make_span() with span(...) constructors.

However you do make a clear case for providing span_FEATURE_MAKE_SPAN, see issue #33 (and #34).

I even run into a situation where 1 part was compiled with C++11 and one with C++14 and make_span got defined in one but not the other but one used the other and got broken.

How is compiling under different C++ standards an (side) effect of span_FEATURE_MAKE_SPAN_TO_STD? Or did it help discover compilation under different C++ standards?

Flamefire commented 5 years ago

Thank you! :+1:

Or did it help discover compilation under different C++ standards?

Yes, but allow me to explain the case:

Now libFoo compiles but exeBar errors because make_span is no longer defined. Of course exeBar could overwrite the define span_FEATURE_MAKE_SPAN_TO_STD=14 but that is a violation of dependency/requirement propagation: libFoo requires make_span, exeBar requires libFoo. ExeBar should not need to care about implementation details of libFoo (which that define is)

I hope you now understand that the only clean solution is that libFoo defines span_FEATURE_MAKE_SPAN=1 to unconditionally add it.

martinmoene commented 5 years ago

Thanks for your helpful explanations!

I hope you now understand that the only clean solution is that libFoo defines span_FEATURE_MAKE_SPAN=1 to unconditionally add it.

The unconditionality could have been simulated via span_FEATURE_MAKE_SPAN_TO_STD=20 to read provide make_span() in the 'foreseeable' future (or even –however undocumented– ...=99).