[INFRA] Remove C++17 support #2915

eseiler · 2021-12-09T16:33:42Z

No description provided.

vercel · 2021-12-09T16:33:47Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/seqan/seqan3/ELLPN8SU1nY2Y7rWvTidNNGcsUug
✅ Preview: https://seqan3-git-fork-eseiler-fix-lambda20-seqan.vercel.app

h-2 · 2021-12-09T17:01:37Z

I think the change is not compatible. The auto syntax does not preserve references / output range, does it?

h-2 · 2021-12-09T17:02:23Z

I thought you dropped GCC7 and GCC8? I explicitly checked that the syntax works with GCC9!

eseiler · 2021-12-09T17:41:02Z

I think the change is not compatible. The auto syntax does not preserve references / output range, does it?

I would need to check what decltype(in) deduces to. But forward seems cleaner anyway as it also makes the intent clear.

I could use a feature test macro to check for template lambdas, and then just use auto as fallback (still not 100% right).
I guess the lambda could also just be an ordinary function?

I thought you dropped GCC7 and GCC8? I explicitly checked that the syntax works with GCC9!

Yes, we dropped GCC7/8, but not yet -std=c++17 -fconcepts; at least not explicitly as far as I am aware.
We still run nightlies on CPP17 mode.
It's something that could be up for discussion, but right now we wouldn't gain much from dropping cpp17 support.

eseiler · 2021-12-09T18:03:44Z

I think I need to write a function object to properly implement this for cpp17.
So, I would go with:

Cpp17: validate_char_for_fn
Cpp20: lambda with template parameter

h-2 · 2021-12-09T18:16:17Z

Hm, so you dropped GCC7 and GCC8, but you did not drop C++17 mode? Do you assume that users have code that is incompatible with C++20? And how will they survive an upgrade of their distro since newer compilers require C++20 mode, or not?

h-2 · 2021-12-09T18:16:54Z

Btw: it is possible to work around this without an extra object but is ugly. See the implementation of views::elements

eseiler · 2021-12-10T10:53:44Z

I put the issue up for discussion on Monday.
The question will be whether to drop cpp17 (explicitly).

smehringer · 2021-12-13T09:52:46Z

Core meeting 13.12.2021

Pro

Very little code is expected to break when a user switches from C++17 to C++20 standard (mostly warnings)
All compilers we support offer C++20
We will never be clang-compatible with C++17 (concepts)
Probably an oversight from dropping GCC7/8

Contra

Some code might break for users
We will keep running ahead of clang when using modern C++20 features

Decision

We will drop C++17 support

Follow-up tasks in this PR:

Remove -std=c++17 detection in CMake
Add warning if user compiles with C++17 (platform.hpp)
Adapt nightlies

eseiler · 2021-12-13T12:35:38Z

The behaviour right now is:

If CMake is used (find_package), we will require CPP20 to be available as 1) builtin, 2) -std=c++20, or 3) -std=c++2a; in this order. Implemented in seqan3-config.cmake.
We will always compile with CPP20 if included via find_package - we set SEQAN3_CXX_FLAGS.
We still need the -fconcepts check for gcc9.
If you only include the headers, there is a warning that we do not support CPP17, see platform.hpp.
This warning can be disabled by defining SEQAN3_DISABLE_CPP17_DIAGNOSTIC.

builtin means that the test-source either compiles without flags, or with the flags set via -DCMAKE_CXX_FLAGS by the user.

`-std=`	`__cplusplus`
c++17	201703
c++2a	201709
c++20	202002, 200204

Documentation will be follow-up. We still need to change docs to reflect the discontinuation of GCC7/8 support.

codecov · 2021-12-13T13:29:06Z

Codecov Report

Merging #2915 (e542712) into master (999352e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2915   +/-   ##
=======================================
  Coverage   98.28%   98.28%           
=======================================
  Files         267      267           
  Lines       11459    11459           
=======================================
  Hits        11262    11262           
  Misses        197      197

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 999352e...e542712. Read the comment docs.

* Add warning in platform.hpp * Add SEQAN3_DISABLE_CPP17_DIAGNOSTIC to disable warning * Only detect CPP20 support

SGSSGene

The future is now!

smehringer · 2021-12-15T18:40:09Z

include/seqan3/core/platform.hpp

+//!\brief This disables the warning you would get if you compile with `-std=c++17`.
+#define SEQAN3_DISABLE_CPP17_DIAGNOSTIC


I would maybe write something a little more directed at the user, like "define this variable to disable..."

Side note: We do not really define this variable, but we need to define it so we can document it?

I just copy-pasted the documentation from the pre-cxx11-abi.
If you want, I can do a doc sweep for these definitions afterwards (new PR) and change all at once.

We only define it for doxygen so that we can document it.
In the actual code, we only check if it is defined, not the value.

smehringer · 2021-12-15T18:42:56Z

include/seqan3/core/platform.hpp

+#   if (__cplusplus < 201703)
+#       error "SeqAn3 requires C++20, make sure that you have set -std=c++2a (gcc9) or -std=c++20 (gcc10 and higher)."
+#   elif not defined(SEQAN3_DISABLE_CPP17_DIAGNOSTIC) && (__cplusplus >= 201703) && (__cplusplus < 201709)
+#      pragma GCC warning "SeqAn 3.1.x is the last version that supports C++17. Newer SeqAn versions, including this one, might not compile with -std=c++17. To disable this warning, use -std=c++2a (gcc9), -std=c++20 (gcc10 and higher), or -DSEQAN3_DISABLE_CPP17_DIAGNOSTIC."


why is this a pragma and not an error?

Same as with pre-cxx-11 ABI.
It might compile, but no guarantees from our side.
It would be totally OK to use cpp17 for now, unless you use the view::char_valid_for or view::assign_strictly_to anywhere.

eseiler requested review from a team and SGSSGene and removed request for a team December 9, 2021 16:33

eseiler removed the request for review from SGSSGene December 9, 2021 18:01

eseiler marked this pull request as draft December 9, 2021 18:01

eseiler changed the title ~~[FIX] Make lambda cpp17 compatible~~ [INFRA] Remove c++17 supoort Dec 13, 2021

eseiler changed the title ~~[INFRA] Remove c++17 supoort~~ [INFRA] Remove c++17 support Dec 13, 2021

eseiler changed the title ~~[INFRA] Remove c++17 support~~ [INFRA] Remove C++17 support Dec 13, 2021

eseiler force-pushed the fix/lambda20 branch 3 times, most recently from 978b527 to b00b4f4 Compare December 13, 2021 12:31

eseiler force-pushed the fix/lambda20 branch 2 times, most recently from 235069a to 15cd07a Compare December 13, 2021 13:04

eseiler marked this pull request as ready for review December 13, 2021 13:19

eseiler requested a review from SGSSGene December 13, 2021 13:19

[INFRA] Remove C++17 support

e542712

* Add warning in platform.hpp * Add SEQAN3_DISABLE_CPP17_DIAGNOSTIC to disable warning * Only detect CPP20 support

eseiler force-pushed the fix/lambda20 branch from 15cd07a to e542712 Compare December 13, 2021 13:56

SGSSGene approved these changes Dec 15, 2021

View reviewed changes

eseiler requested a review from smehringer December 15, 2021 13:06

This was linked to issues Dec 15, 2021

[CRON] Latest Libraries #2747

Closed

[CRON] AVX2 #2905

Closed

smehringer requested changes Dec 15, 2021

View reviewed changes

eseiler requested a review from smehringer December 16, 2021 09:45

smehringer approved these changes Dec 17, 2021

View reviewed changes

smehringer merged commit cf4e1d7 into seqan:master Dec 17, 2021

eseiler deleted the fix/lambda20 branch December 17, 2021 08:56

eseiler removed a link to an issue Apr 24, 2022

[CRON] Latest Libraries #2747

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[INFRA] Remove C++17 support #2915

[INFRA] Remove C++17 support #2915

eseiler commented Dec 9, 2021 •

edited

Loading

vercel bot commented Dec 9, 2021 •

edited

Loading

h-2 commented Dec 9, 2021

h-2 commented Dec 9, 2021

eseiler commented Dec 9, 2021 •

edited

Loading

eseiler commented Dec 9, 2021

h-2 commented Dec 9, 2021

h-2 commented Dec 9, 2021

eseiler commented Dec 10, 2021

smehringer commented Dec 13, 2021 •

edited by eseiler

Loading

eseiler commented Dec 13, 2021 •

edited

Loading

codecov bot commented Dec 13, 2021 •

edited

Loading

SGSSGene left a comment

smehringer Dec 15, 2021

eseiler Dec 16, 2021

smehringer Dec 15, 2021

eseiler Dec 16, 2021

		//!\brief This disables the warning you would get if you compile with `-std=c++17`.
		#define SEQAN3_DISABLE_CPP17_DIAGNOSTIC

[INFRA] Remove C++17 support #2915

[INFRA] Remove C++17 support #2915

Conversation

eseiler commented Dec 9, 2021 • edited Loading

vercel bot commented Dec 9, 2021 • edited Loading

h-2 commented Dec 9, 2021

h-2 commented Dec 9, 2021

eseiler commented Dec 9, 2021 • edited Loading

eseiler commented Dec 9, 2021

h-2 commented Dec 9, 2021

h-2 commented Dec 9, 2021

eseiler commented Dec 10, 2021

smehringer commented Dec 13, 2021 • edited by eseiler Loading

Core meeting 13.12.2021

Pro

Contra

Decision

eseiler commented Dec 13, 2021 • edited Loading

codecov bot commented Dec 13, 2021 • edited Loading

Codecov Report

SGSSGene left a comment

Choose a reason for hiding this comment

smehringer Dec 15, 2021

Choose a reason for hiding this comment

eseiler Dec 16, 2021

Choose a reason for hiding this comment

smehringer Dec 15, 2021

Choose a reason for hiding this comment

eseiler Dec 16, 2021

Choose a reason for hiding this comment

eseiler commented Dec 9, 2021 •

edited

Loading

vercel bot commented Dec 9, 2021 •

edited

Loading

eseiler commented Dec 9, 2021 •

edited

Loading

smehringer commented Dec 13, 2021 •

edited by eseiler

Loading

eseiler commented Dec 13, 2021 •

edited

Loading

codecov bot commented Dec 13, 2021 •

edited

Loading