Set minimum required C++ version to C++17

[ComicSansMS]

This is required for the nested namespace specifiers in identity.hpp.

Without this change, the example will fail to compile on MSVC.
If it is not desirable to have a C++17 as the minimum version, the example code should be changed to not rely on any C++17 features instead.

[camio]

beman.exemplar's build instructions suggest using -DCMAKE_CXX_STANDARD=20 on the command line. If you do this, does the Visual C++ build work?

The reason we aren't using target_compile_features so far is because it could have the side-effect of changing compiler flags of applications depending on that library. This makes it difficult for the top-level application owner to control which language compiler flags to use. You can find a discussion of that at beman-project/inplace_vector#6 (comment)

I'm beginning to think we need a try_compile call that'll give the user a good error message if the compiler isn't sufficient (e.g. suggesting using -DCMAKE_CXX_STANDARD).

[ComicSansMS]

beman.exemplar's build instructions suggest using -DCMAKE_CXX_STANDARD=20 on the command line. If you do this, does the Visual C++ build work?

Yes, that will solve it. CMAKE_CXX_STANDARD will populate the compile features property for all targets.

The reason we aren't using target_compile_features so far is because it could have the side-effect of changing compiler flags of applications depending on that library. This makes it difficult for the top-level application owner to control which language compiler flags to use. You can find a discussion of that at beman-project/inplace_vector#6 (comment)

I don't quite follow the rationale here. You have a de-facto PUBLIC requirement on the language version here, whether you advertise it through the build system or not. Clients that are not at least using C++17 will not be able to consume the library.
Imho, if clients want to ensure that they always compile with a specific language version, that is the responsibility of the client's build system, not of beman. I don't see any benefit in pulling this option to the command line, when it is a mandatory option and forgetting to specify it will just stop the library from building. Imho, command line options should be used for optional things, e.g. whether you want to build with or without tests enabled, not mandatory ones.

[steve-downey]

Because letting packages change the ABI of the consumer breaks package management. The model Cmake uses only works, to the extent that it does, if every package is built as part of the same project, and even then it produces surprises.

Packages must build with the ABI they are given, and fail if they can't be built using those flags. Failure is an option in package management.
Standard level is part of ABI, and feature flags are part of that, particularly where they are used to define polyfills.

But also changes like adding members to std::string have had ABI changes in real implementations, such as RH's developer toolset, because it means the full template instantiation can't be used from the existing system libraries, and instead must be emitted, which means that symbol resolution changes cause link changes.

[steve-downey]

We're also a bit torn here between extra complications that a simple example doesn't need, and a full project does. Optional26 specifies this, and other flags, as part of the toolchain file used for the build. That's the mechanism that package management systems often use to provide build instructions, if they don't replace the package build system wholesale.

[neatudarius]

If we want to require a minimum version, it should be C++20 due to this example https://github.com/beman-project/exemplar/blob/main/examples/CMakeLists.txt#L4. I'll re-enable it - TLDR exemplar requires range support (https://en.cppreference.com/w/cpp/algorithm/ranges). Apologies for this inconvenience and thanks @ComicSansMS for contributing!

Just created #41

[camio]

If we want to require a minimum version, it should be C++20

If we do that, I'm afraid we'll cut off several developers who are still on 17. Should we selectively enable the test based on a try_compile result?

[steve-downey]

Given the mission of the project I'd say that 20 is a bare minimum for working on anything in Beman. In particular the implementation techniques are radically different for standards proposals pre-20 and post. Requires clauses make a huge difference in rendering a library.

[neatudarius]

=============================================

The production code and tests (everything that is in include/ and src) should be C++17 compatible. @ComicSansMS , would you mind to temporary push 2 lines here (gcc/clang + c++17) https://github.com/beman-project/exemplar/blob/main/.github/workflows/ci_tests.yml#L26-L31, to be clear for everybody what all C++ code compiles with C++17, except this file https://github.com/beman-project/exemplar/blob/main/examples/identity_as_default_projection.cpp?

=============================================

I also think setting C++20 is a normal constraint for Beman repos. However, I'm not opposing to discuss further this approach vs try_compile.

Should we selectively enable the test based on a try_compile result?
just to be clear: @camio , is not a test, it's an example. Basically, we have 2 examples: one compatible with C++17, and one which needs C++20 (ranges support).

Possibilities here:

1. Want to keep relevant usage with default projection for ranges algorithms <=> require C++20 or later.
2. Want to keep relevant usage with default projection for ranges algorithms <=> require C++17 (use try_compile for this file)
3. We don't want this example (which is already known from https://en.cppreference.com/w/cpp/utility/functional/identity) => we remove it, we can set C++17 as minimum required version.

[wusatosi]

I also think setting C++20 is a normal constraint for Beman repos. However, I'm not opposing to discuss further this approach vs try_compile.

[One example needs C++20, but the project can be build with C++17]
[Matrix of when to enable C++20 and when to use C++17 only, we could use try_compile to selectively enable]

I am for setting C++20 as the minimal C++ version. We are building libraries that's meant to be included to the HEAD of standard library, I do not see why we couldn't use C++20.

[camio]

We are building libraries that's meant to be included to the HEAD of standard library, I do not see why we couldn't use C++20.

It's a tradeoff. One of the goals of the Beman project is to collect user feedback on proposed standard libraries before they land in the standard. Requiring newer compilers will block several folks, especially those in large companies, from getting that feedback. At Adobe, for example, not all our compilers support C++20 yet.

That being said, if there's a huge implementation burden to supporting older compilers for a particular library then it makes sense to require something newer. I'd take it on a case-by-case basis.

[steve-downey]

Expressing Constraints clauses is the big one. Anything other than a constraint-expression is an implementation burden.

https://timsong-cpp.github.io/cppwp/n4861/structure#specifications-3.1

Constexpr if is also the natural way of expressing some of the requirements in the library descriptions.

There are other ways of doing things, but often not easily or well.

It's of course case-by-case, but don't be surprised if almost all are C++20 minimum.

Compare libc++ optional with Optional26.

[wusatosi]

Previously requested change completed in #55 .

We had consensus on exemplar supporting C++ 17 during today's weekly meeting thus this pr should move forward.

[camio]

My understanding is that the original motivation for this PR is that the build would fail when you don't set your compilation flags appropriately with something like -DCMAKE_CXX_STANDARD=17. Now that we're using presets, I think this is largely solved. Correct?

Adding target_compile_features(beman.exemplar PUBLIC cxx_std_17) is problematic because it can add flags to dependent projects which is undesirable.

If I understand correctly, this PR can now be closed with merging since the original concern is addressed. @ComicSansMS, did I get this right?

[neatudarius]

LGTM, if we still want to add the config in that file. C++17 is accepted right now.

[camio]

Closing because presets already address this concern and don't have the drawback of changing dependent compiler flags. @ComicSansMS or others please feel free to reopen if you disagree.