span-lite icon indicating copy to clipboard operation
span-lite copied to clipboard

span_FEATURE_*_TO_STD does not do what is advertised

Open Flamefire opened this issue 5 years ago • 7 comments

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)

Flamefire avatar Jan 15 '19 16:01 Flamefire

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 avatar Jan 15 '19 22:01 martinmoene

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

martinmoene avatar Jan 15 '19 23:01 martinmoene

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

Flamefire avatar Jan 16 '19 07:01 Flamefire

Fix of span_IN_STD:

#define span_IN_STD( v )  ( ((v) == 98 ? 3 : (v)) >= span_CPLUSPLUS_V )

martinmoene avatar Jan 16 '19 09:01 martinmoene

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?

martinmoene avatar Jan 16 '19 09:01 martinmoene

Thank you! :+1:

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

Yes, but allow me to explain the case:

  • libFoo has a header foo.hpp including nostd/span and some sources
  • this header uses make_span in some templated function
  • CMakelists of libFoo contains target_compile_features(std_cxx_11) as this is what is required. It also sets target_compile_options(span_FEATURE_MAKE_SPAN_TO_STD=11)
  • Target exeBar consumes libFoo but requires C++14 so it sets target_compile_features(std_cxx_14)

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.

Flamefire avatar Jan 16 '19 10:01 Flamefire

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).

martinmoene avatar Jan 16 '19 10:01 martinmoene