C++14 requirement mismatch between README and source code

[oliverlee]

README says:
C++14 backport (e.g., fold expressions not required)

but I noticed use of fold expressions here:

mdspan/include/experimental/__p0009_bits/extents.hpp

Lines 61 to 63 in fef0c8a

	return
	(std::is_convertible<Arguments, IndexType>::value && ... && true) &&
	(std::is_nothrow_constructible<IndexType, Arguments>::value && ... && true);

there may be more (I haven't checked).

Should the use of fold expressions be removed? Or will C++14 support require some C++17 language features?

[mhoemmen]

I think the plan is to require C++17. Do you need C++14 support?

[oliverlee]

The project(s) that use this currently support C++14. I would appreciate some form of C++14 support if possible (maybe don't use submdspan and allow C++17 extensions?).

[mhoemmen]

mdspan has macros for C++14 compatibility. (EXPR && ... && true) should mean the same thing as (EXPR && ...), so one could replace each of those two fold expressions with appropriate use of _MDSPAN_FOLD_AND, which is defined in macros.hpp. Would you be interested in submitting a pull request?

[oliverlee]

Sure I can try and do that in the next few days. Should I update #311 to be a general C++14 fixup PR or should I submit a separate PR?

[mhoemmen]

@oliverlee Since lambda expressions aren't allowed in constant expressions until C++17, PR #311 probably wouldn't be useful without more C++14 fixes, so yes, I think it's reasonable to update PR #311 rather than creating a new PR.

That being said, you might prefer to consult with the other maintainers of this repository -- @crtrott , @dalg24 , @nmm0 , and others -- before setting out to do a lot of work.

[oliverlee]

That PR seems to be sufficient for C++14 support as we allow C++17 extensions specifically in mdspan and avoid use of submdspan.

I'm happy to make some smaller changes (e.g. replacing fold expressions with a macro). I don't mind doing some other things (e.g. replacing lambda expressions, if constexpr) as long as you want those sort of changes.

[mhoemmen]

@oliverlee wrote:

That PR seems to be sufficient for C++14 support as we allow C++17 extensions specifically in mdspan and avoid use of submdspan.

Thanks for clarifying! This is good to know. If that PR #311 suffices for C++14 support for you, we can merge it.

I'm happy to make some smaller changes (e.g. replacing fold expressions with a macro). I don't mind doing some other things (e.g. replacing lambda expressions, if constexpr) as long as you want those sort of changes.

We generally have been considering C++14 support a legacy feature that we don't intend to maintain with new code. For example, we only test with C++17 and newer language versions (see https://github.com/kokkos/mdspan/blob/stable/.github/workflows/cmake.yml ).

If you're interested in helping to support a partial C++14 back-port, we should talk more about what features you would like to back-port and how you intend to test it. @crtrott et al. would need to agree and we would have to partition the tests based on minimum required C++ version.

[oliverlee]

Thanks for clarifying! This is good to know. If that PR #311 suffices for C++14 support for you, we can merge it.

Sounds good to me. Perhaps cleaning up fold expressions and other C++14 support items could be done in a separate PR after figuring out what exactly that entails?

[mhoemmen]

@oliverlee wrote:

Perhaps cleaning up fold expressions and other C++14 support items could be done in a separate PR after figuring out what exactly that entails?

I think that's a good idea. We may not actually want to support C++14 if it's too invasive of a change or if it makes the code too much harder to read.

FYI, our 2019 mdspan paper talks about how compiling with C++17 instead of C++14 improved performance of benchmarks.

Thanks! : - )

[oliverlee]

Let me know if just replacing fold expressions makes sense. Or perhaps it would be better to remove note in the README about C++14 support.

I'm not surprised that compile times are better with C++17 - unfortunately we're still working with C++14 for the forseable future.

[mhoemmen]

@oliverlee What compilers and what versions of those compilers do you need to support?

I can't speak for @crtrott et al., but I'd like to see how big and invasive of a change this would entail, before contemplating promising support. For example, fold expressions would need to be replaced with appropriate macros (that we already have in our macros.hpp header), so that we don't lose performance for C++17 use cases.

[oliverlee]

We currently test with llvm 17, gcc 13, and arm-none-eabi-gcc 12. In the future, I assume we will need to support older compilers that may not have implemented C++17 language features.

As a first pass, I can probably start with replacing fold expressions with a macro. I don't have a good idea about the impact on performance for other changes - I assume that replacing if constexpr with function dispatch will result in slower compile times.

[crtrott]

Hi,

I think we could make a compromise here if you are willing to help maintain that part, and say that mdspan as in C++23 (i.e. mdspan, extents, layout_left, layout_right and layout_stride) support C++14 but everything else doesn't.

Christian

[oliverlee]

So just the parts from P0009 would be in scope? If that's the case, sure.