Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

span_FEATURE_*_TO_STD does not do what is advertised #32

Open
Flamefire opened this issue Jan 15, 2019 · 7 comments
Open

span_FEATURE_*_TO_STD does not do what is advertised #32

Flamefire opened this issue Jan 15, 2019 · 7 comments

Comments

@Flamefire
Copy link

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
Copy link
Owner

martinmoene commented Jan 15, 2019

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 added a commit that referenced this issue Jan 15, 2019
@martinmoene
Copy link
Owner

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

@Flamefire
Copy link
Author

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
Copy link
Owner

Fix of span_IN_STD:

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

@martinmoene
Copy link
Owner

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
Copy link
Author

Thank you! 👍

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.

@martinmoene
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants