Using C++17 features

Is there any objection to using C++17 features if they are available?

I’m willing to begin using C++17 (and C++14) features to replace existing code if we use feature tests for and fall back to our current code when features are not available.

Summaries of what compiler versions support various C++14 and C++17 features:

Allowing to use C++17 STL features (conditionally), would most likely allow simplifying the code that we use for canonicalize_file_name on Windows significantly.
As a side note: IIUC, we’d need to compile with C++17 support anyway if we’d like to start using Qt6.

I’m not sure how to correctly read the compiler support page:
C++ compiler support - cppreference.com
It mentions which versions of clang and gcc completed the support for a set of features (core or library). But I’m assuming that older compilers would complain if we were to set -std=c++17. Which versions would that be?

We would have to test to see whether there is an option like -std=c++17 that works and whether it is needs to be used to enable C++17 features. To test that, we could try to compile a few features that we will be using, first with no option, then with whatever options are used (do all compilers use the -std=c++N option syntax?). Then in addition to testing for the option, we can test for individual library features as needed.

I found ax_cxx_compile_stdcxx (Autoconf Archive) which looks like it implements a check for C++17 compatible compilers.
I went ahead and made what I think were the necessary changes to require C++17 in the build system.

The attached patch does that and also adds a test if the filesystem API can be used.
It also uses the filesystem API for our implementation of canonicalize_file_name if possible.

With those changes Octave compiles for me on Ubuntu 20.10 (gcc 10.2.0) and the test suite still passed.

I hope that this will resolve the last major file system issues we have on Windows. But I haven’t tested yet.

If we should decide to go ahead with this, it would probably make sense to split that patch into meaningful pieces.

cxx17.patch (27.3 KB)

Two typos in that patch.
Hopefully fixed in this one: cxx17_v2.patch (27.3 KB)

1 Like

I tried to cross-compile for Windows with those changes including Octave Forge packages.
It looks like at list one of the ones included in MXE Octave isn’t compatible to C++17.
For of-geometry, compilation ends with this in the logs:

x86_64-w64-mingw32-g++ -std=gnu++17 -c -I/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include  -I/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-7.0.0/octave/.. -I/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-7.0.0/octave -I/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include   -fopenmp -g -O2    martinez.cpp -o martinez.o
In file included from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/lib/gcc/x86_64-w64-mingw32/10.2.0/include/c++/set:60,
                 from martinez.h:19,
                 from martinez.cpp:9:
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/lib/gcc/x86_64-w64-mingw32/10.2.0/include/c++/bits/stl_tree.h: In instantiation of 'static const _Key& std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::_S_key(std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::_Const_Link_type) [with _Key = Martinez::SweepEvent*; _Val = Martinez::SweepEvent*; _KeyOfValue = std::_Identity<Martinez::SweepEvent*>; _Compare = Martinez::SegmentComp; _Alloc = std::allocator<Martinez::SweepEvent*>; std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::_Const_Link_type = const std::_Rb_tree_node<Martinez::SweepEvent*>*]':
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/lib/gcc/x86_64-w64-mingw32/10.2.0/include/c++/bits/stl_tree.h:2101:47:   required from 'std::pair<std::_Rb_tree_node_base*, std::_Rb_tree_node_base*> std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::_M_get_insert_unique_pos(const key_type&) [with _Key = Martinez::SweepEvent*; _Val = Martinez::SweepEvent*; _KeyOfValue = std::_Identity<Martinez::SweepEvent*>; _Compare = Martinez::SegmentComp; _Alloc = std::allocator<Martinez::SweepEvent*>; std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::key_type = Martinez::SweepEvent*]'
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/lib/gcc/x86_64-w64-mingw32/10.2.0/include/c++/bits/stl_tree.h:2154:4:   required from 'std::pair<std::_Rb_tree_iterator<_Val>, bool> std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::_M_insert_unique(_Arg&&) [with _Arg = Martinez::SweepEvent* const&; _Key = Martinez::SweepEvent*; _Val = Martinez::SweepEvent*; _KeyOfValue = std::_Identity<Martinez::SweepEvent*>; _Compare = Martinez::SegmentComp; _Alloc = std::allocator<Martinez::SweepEvent*>]'
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/lib/gcc/x86_64-w64-mingw32/10.2.0/include/c++/bits/stl_set.h:512:25:   required from 'std::pair<typename std::_Rb_tree<_Key, _Key, std::_Identity<_Tp>, _Compare, typename __gnu_cxx::__alloc_traits<_Allocator>::rebind<_Key>::other>::const_iterator, bool> std::set<_Key, _Compare, _Alloc>::insert(const value_type&) [with _Key = Martinez::SweepEvent*; _Compare = Martinez::SegmentComp; _Alloc = std::allocator<Martinez::SweepEvent*>; typename std::_Rb_tree<_Key, _Key, std::_Identity<_Tp>, _Compare, typename __gnu_cxx::__alloc_traits<_Allocator>::rebind<_Key>::other>::const_iterator = std::_Rb_tree<Martinez::SweepEvent*, Martinez::SweepEvent*, std::_Identity<Martinez::SweepEvent*>, Martinez::SegmentComp, std::allocator<Martinez::SweepEvent*> >::const_iterator; std::set<_Key, _Compare, _Alloc>::value_type = Martinez::SweepEvent*]'
martinez.cpp:137:29:   required from here
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/lib/gcc/x86_64-w64-mingw32/10.2.0/include/c++/bits/stl_tree.h:780:8: error: static assertion failed: comparison object must be invocable as const
  780 |        is_invocable_v<const _Compare&, const _Key&, const _Key&>,
      |        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
make[2]: *** [Makefile:15: martinez.o] Error 1
make[2]: Leaving directory '/home/osboxes/Documents/Repositories/Octave/mxe-octave/tmp-of-geometry/tmpcj85bv2q-pkg/geometry-4.0.0/src'

It is probably a good thing that the newer standard is stricter in some respect. But this probably also means that some user code that worked “just fine” before C++17 might fail to compile if we decide to make that change.

Edit: I used this patch to allow it to compile with C++17: of-geometry-1-cxx17.patch (3.3 KB)

Seems like we might end up with substantial amounts of parallel code. As long as we’re willing to test for features, we could even include C++20. If a particular crucial piece of code had seen substantial improvement over time in the libraries then you might end up with 4 implementations for C++11, C++14, C++17, and C++20. That is worst case, but in general there could be a lot of divergent code paths.

Playing devil’s advocate, we already have a lot of #ifdefs that are used to support Windows. If we could replace those with a test for a C++ version and then use common code for both Linux & Windows then the overall number of #ifdefs would be the same.

Fwiw, the patch above allows me to execute files that are in paths that contain characters that aren’t encoded in the locale charset on Windows. Both in the working directory or in the search path.

I might be misunderstanding. But I thought the C++17 standard is like the baseline how the compiler interprets our code. Switching to C++20 might mean that our code (and fwiw user code) will be interpreted differently in places.

I agree that it would be a good thing if we could unconditionally use the new features. That would indeed reduce the complexity of our code. But e.g. for the filesystem API, support in the compilers is still quite new. IIUC, it only finally made it into gcc version 10 (for gcc 8 and 9 it could be used with additional linker switches). Similarly for clang.
So I think that — at least for the time being — it is probably reasonable to still have fallback code around in that particular case.

I mostly meant this idea: that the Standard Template Library (STL) adds new features over time and that we might need to check versions of the compiler to see whether we could use those features. If the compiler/STL that we are using is old, say C++11, then there would be features that we would have to code workarounds for.

There is an additional idea that some features which are common to all versions of C++ have, nevertheless, changed functionality over time. For example, in C++03, the std::string implementation is not guaranteed to store characters contiguously in memory nor is the data guaranteed to be null-terminated like a C-string. For that reason, if you want a C string a programmer must use the member function c_str(). On the other hand, in C++11 both of those conditions are now guaranteed. So this code is now legal

std::string mystr ("Hello World");
char *str_ptr = &mystr[0];

The code snippet in comment #4 is another example for how the implementation has changed between C++11 and C++17. In that case, it was about const-ness of the operator() of a type used in a std::set.

You are right that it might make sense to check if certain features are supported by the compiler independent of which standard we end up using. I also agree that features in the STL are probably more likely to be missing for any given compiler than (common) core features.

I don’t think our goal should be to be able to maintain Octave’s sources for several C++ standards at the same time. Instead, we should decide which standard we’d like to adopt. (I’d vote for C++17 if we’d like to change the current standard. See below.)
That could still mean that it might make sense to have configuration checks for certain features of that standard. And as a consequence maybe also some alternative implementation. At least until we are confident that that feature is supported commonly enough.

IIUC, C++14 and C++17 were just “minor” updates to C++11. There will probably be just a few cases (but there will be) where existing code will break if we moved to one of those standards.
It’s probably more likely that existing code will break if we switched to C++20 now. IIUC, that update made more radical changes to the standard.

I found an alternative implementation for bug #60306 that doesn’t require C++17.
So at least for that use case, requiring a newer C++ dialect is less pressing.

In case we’d still like to switch to C++17, here is a patch that should apply on a current tip: cxx17_v3.patch (26.9 KB)