Planning for C++17 migration

[JohanMabille]

This issue tracks the benefits of migrating the codebase to C++17, and its consequences.

First we would have to drop the support for the following compilers:

• gcc 4.9, 5, 6
• clang 5
• MSVC 2015

We may also check that this migration does not break downstream packages, especially those depending on libraries we do not maintain. The downstream packages to check are:

• xtensor-python (depends on pybind11)
• xtensor-r (depends on RCPP)
• xtensor-julia has not been maintained for a while because it depends on cxx_wrap which has upgraded to C++17. Upgrading xtensor to C++17 may fix the issues there.
• xtensor-io (depends on a lot of io-libraries)
• xtensor-blas (checks that everything is fine with different BLAS implementations)
• xtensor-fftw

That being said, here are the new features of C++17 that xtensor could take advantage of:

• if constexpr: we could replace the usage of the static_if from the xtl, as well as the pattern based on private SFINAE methods.
• class template argument deductions: a lot of building functions whose unique role is to deduce template arguments could be removed.
• constexpr lambda, fold expressions, lambda capture of *this and inline variables would be useful too, but they would have a less significant impact.
• Dropping support for MSVC 2015 means that we could reintroduce support for xtensor_fixed on Windows.
• new functions available in the STL (std::size)

cc @SylvainCorlay @tdegeus @wolfv who may have other ideas or remarks.

[tdegeus]

I'm principle I'm always using reasonable modern feature: Why spend many lines of code during several years working around something that is fixed. That being said... Indeed the end-user should be not be stuck with an unworkable set of libraries, some needing C++17, some being broke with it. There I might be naive, but it is not so much the pre-compiled libraries (I think that linking can be done between different standards) but rather the header-only ones? (Ok, they are probably sufficient modern). Maybe we should have a list here what which libraries we should verify?

[JohanMabille]

Indeed, it's not especially the compiled libraries, it's more that these are the libraries we usually don't maintain (in the xtensor-stack). I will edit the original post to list them.

I remember that at some point @wolfv set up an automatic build of downstream packages upon xtensor commit. It was running on a machine at QuantStack. We could create a repo for doing that on a CI, but I wonder if we could have it checking the build automatically. But even if we lose that automatic trigger, I find it more sustainable to have it on the cloud rather than on a local machine.

[emmenlau]

Just a "thumbs up" from us. We've mostly switched to C++17 on all platforms and we're pretty happy with the mileage we're getting!

[davidbrochart]

As @JohanMabille mentioned, another reason for switching to C++17 is using https://github.com/wjakob/nanobind in xtensor-python instead of pybind11.

[spectre-ns]

@JohanMabille Boost maintains compatibility by using many very ugly macro #ifdef statements to establish what standard (and sometime the compiler vendor and version) is being used on that compile. We have moved to C++17 in our organization as well. std::visit and if constexpr were the features we needed. Seems that under the hood there is no advantage to compiling in newer standards but you could likely make the interfaces more expressive using more modern standards and the associated syntax.

[tdegeus]

There are plenty of reason to switch to C++, but here is one more for an xtensor user who wants to wrap e.g. with xtensor-python. Ideal is:

template <class R = xt::xarray<double>>
R myfunc();

In C++17 this can be used as 'normal':

auto a = myfunc(); 

while in Python there is no extra copy:

m.def("myfunc", &myfunc<xt::parray<double>>);

Instead, with C++14 the end-user is stuck with ugly code:

auto a = myfunc<>(); 

with the unnecessarily <> interfering.

[spectre-ns]

There are plenty of reason to switch to C++, but here is one more for an xtensor user who wants to wrap e.g. with xtensor-python. Ideal is:

template <class R = xt::xarray<double>>
R myfunc();

In C++17 this can be used as 'normal':

auto a = myfunc(); 

while in Python there is no extra copy:

m.def("myfunc", &myfunc<xt::parray<double>>);

Instead, with C++14 the end-user is stuck with ugly code:

auto a = myfunc<>(); 

with the unnecessarily <> interfering.

This also true when using numeric_constants<>::PI

[emmenlau]

Just curious if its possible to support c++17 and c++20 in one go? We moved on to c++20 on all platforms and there are a number of constructs that are quite helpful. I'm specifically not asking that xtensor uses c++20, but only full compatibility with a c++20 build.

[spectre-ns]

Also the easing of the typename keyword requirements in C++20 would be awesome for writing metaprogramming code.

[spectre-ns]

@JohanMabille @tdegeus I'm trying to implement the interface below for scipy find peaks into xtensor. Do you have any tips on how to implement this without having to use std::enable_if everwhere? It's going to be super verbose if I have to write a new function for each part of the control path based on if I have none types or actual values the way scipy does it. if constexpr allows me to basically write this like a normal function using std::same_as<E1, E2>()::value with the control path being decided at compile time.

Are you planning to migrate to C++17 and start using those features or are you going to just make sure that it can compile using C++17 while maintaining backwards compatibility with C++14?

      /*
        * @brief finds all peaks and applies filters to the peaks based on the parameters provided.
        * @detail portions of this function are unimplemented and will throw a compiler error
        *   if features requested are not available.
        * @param 1D data vector
        * @param minimum height thresholds to be filtered. Only minimum heights are implemented
        * @param threshold unimplemented
        * @param distance unimplemented
        * @param prominence unimplemented
        * @param width defines the minimum width for eligable points. if a second parameter is applied a max width is also applied
        * @param wlen defined wlen for calculating prominence
        * @param rel_height used in defining the width of peaks based on the location. Defaults to .5 or 6dB
        * @param plateau_size unimplemented
        */

        template <
            class E1,
            class E2 = decltype(xt::xnone()),
            class E3 = decltype(xt::xnone()),
            class E4 = decltype(xt::xnone()),
            class E5 = decltype(xt::xnone()),
            class E6 = decltype(xt::xnone()),
            class E7 = decltype(xt::xnone()),
            class E8 = double,
            class E9 = decltype(xt::xnone())
        >

            inline auto find_peaks(
                E1&& x,
                E2&& height = xt::xnone(),
                E3&& threshold = xt::xnone(),
                E4&& distance = xt::xnone(),
                E5&& prominence = xt::xnone(),
                E6&& width = xt::xnone(),
                E7&& wlen = xt::xnone(),
                E8&& rel_height = .5,
                E9&& plateau_size = xt::xnone())
{}

[tdegeus]

Concerning the C++17 migration. I'm not sure. Personally, I'm fine with changing entirely to C++17, not dragging along limitations that are no longer there. But the reality in HPC is that you might be on relatively old compilers, or there may be this one dependency that is still C++17 incompatible. So surely it should be two steps: 1) ensuring that xtensor compiles on C++17 in one version, and 2) dropping C++14 support in a next version.

Concerning your function. Having many keyword arguments is a fantastic feature on Python, and honestly a big limitation of C++ that if I were in charge would have solved ;). For readability maybe it is best to consider a struct find_peaks_settings where the user can select its options (from defaults). That would increase the verbosity on the user side (which is good I think if there are so many options), and keeps the possibility of future API changes.

[AntoinePrv]

Moving to C++17 also offers some more mathematical functions. https://en.cppreference.com/w/cpp/header/cmath

[tdegeus]

I lost a track of where we are at in terms of the C++17 support/migration. I have been over to C++17 for a while now on my research codes, and I must say that especially constexpr is just a blessing in terms of readability/maintainability. I think that this advantage would be even much greater here.

I would say that all major compilers have been supporting C++17 for an ample number of releases, such that this should not be our worry.

Then:

1. We have to make sure that at least the xtensor-suite remains operable. This is in the end testing issue, where we pay for only running tests against releases.
2. Users can have blocking other dependencies. However, until we move to C++17 we will never find out.

Since xtensor and many other codes in the xtensor-suite have been quite stable, I propose that we opt for new major releases of the xtensor-suite that all use C++17, and offer a fixed finite life-time bugfix support of the current major releases. I would say that instead of worrying too much, we just make the switch and deal with any issues from there (like stated, I think that the current releases are quite stable enough, and we can always release minor bugfixes if needed).

The only discussion point could be if we should not immediately step to C++20 and solve issues only once and for a while to come

[spectre-ns]

I lost a track of where we are at in terms of the C++17 support/migration. I have been over to C++17 for a while now on my research codes, and I must say that especially constexpr is just a blessing in terms of readability/maintainability. I think that this advantage would be even much greater here.

I would say that all major compilers have been supporting C++17 for an ample number of releases, such that this should not be our worry.

Then:

1. We have to make sure that at least the xtensor-suite remains operable. This is in the end testing issue, where we pay for only running tests against releases.
2. Users can have blocking other dependencies. However, until we move to C++17 we will never find out.

Since xtensor and many other codes in the xtensor-suite have been quite stable, I propose that we opt for new major releases of the xtensor-suite that all use C++17, and offer a fixed finite life-time bugfix support of the current major releases. I would say that instead of worrying too much, we just make the switch and deal with any issues from there (like stated, I think that the current releases are quite stable enough, and we can always release minor bugfixes if needed).

The only discussion point could be if we should not immediately step to C++20 and solve issues only once and for a while to come

I agree. I've run into several instances where static_if from the xtl is inferior to if constexpr. I would vote to jump to C++20... Concepts would make the interface and errors infinity nicer.