Change C++ version to C++17 #7989

esteffin · 2023-10-31T16:36:01Z

Changed gnu-make and cmake-based builds to C++17.

Each commit message has a non-empty body, explaining why the change was made.
Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
My commit message includes data points confirming performance improvements (if claimed).
My PR is restricted to a single feature or bugfix.
White-space or formatting changes outside the feature-related changed lines are in commits of their own.

codecov · 2023-10-31T17:59:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9238a38) 77.75% compared to head (032f68c) 79.09%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #7989      +/-   ##
===========================================
+ Coverage    77.75%   79.09%   +1.33%     
===========================================
  Files         1701     1699       -2     
  Lines       196465   196471       +6     
===========================================
+ Hits        152762   155390    +2628     
+ Misses       43703    41081    -2622

see 125 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tautschnig · 2023-11-01T10:42:39Z

What about #7172?

tautschnig · 2023-11-01T16:36:07Z

src/goto-instrument/document_properties.cpp

@@ -238,7 +238,7 @@ document_propertiest::get_code(const source_locationt &source_location)
      while(line_no.size()<4)
        line_no=" "+line_no;

-      line_no+"  ";
+      line_no += "  ";


Seems unrelated?!

I agree that this is worth separating into its own PR. Some of the new versions of the standard library add the [[nodiscard]] attribute to the + operator of std::string. This causes a compile error when the result of the concatenation is unused. It essentially promotes a logic error into a compile error. Without this commit, the check-ubuntu-22_04-cmake-gcc-13 job will fail, so it does appear to be needed before we can switch standards. However as it is a logic error currently we could fix it before switching standards.

That is a fix for an issue brought up by the C++17 change as the operator+ between strings is marked nodiscard and so it was resulting in a compilation error.

If it is too awkward to include this here, I'm happy to move the change to a separate PR, but I would need it in before upgrading the standard.

I think this should be PR'ed and fixed right away. (I guess no one is using that feature so it went unnoticed, but it should really be fixed now.)

@tautschnig I just read the surrounding code and the value of line_no is never read back and output. So it is just as broken before and after this commit...

Fair point. I suspect this should have been inserted as a prefix into each line of output. Let me make a stab at a PR.

Fix in #7994.

I will remove this commit from this PR as soon as the latter is merged.

martin-cs · 2023-11-02T11:43:39Z

We have traditionally been very conservative about C++ version increments as it is not uncommon to find the "approved" Linux platform some users have trailing the current version by as much as 5 years. Ubuntu 18.04 LTS is only just out of general support, still gets security fixes and I strongly suspect still has quite a user base. Generally supporting an older C++ version is a good thing. But, it can be worth increasing the version if it makes the project better in some way. From this PR it is not clear to me what the benefits are. There are probably some that I just don't see. Can anyone say what they are?

thomasspriggs · 2023-11-02T13:08:02Z

We have traditionally been very conservative about C++ version increments as it is not uncommon to find the "approved" Linux platform some users have trailing the current version by as much as 5 years. Ubuntu 18.04 LTS is only just out of general support, still gets security fixes and I strongly suspect still has quite a user base. Generally supporting an older C++ version is a good thing. But, it can be worth increasing the version if it makes the project better in some way. From this PR it is not clear to me what the benefits are. There are probably some that I just don't see. Can anyone say what they are?

@martin-cs Just a reminder - you approved the previous PR making the equivalent changes and you seemed satisfied with the motivating factors at the time - #7172 (review)

A quick summary of some of the advantages off the top of my head -

Removing the library code which has been back ported C++11, which will reduce the maintenance needed.
Using standard library versions of code lowers the barriers to entry for experienced C++ developers who are not yet familiar with the cbmc. Currently contributors who fall into this category have to learn details such as use optionalt instead of std::optional.
Access to new C++17 STL classes such as std::variant.

martin-cs · 2023-11-02T14:48:04Z

@thomasspriggs thanks for the reminder; had totally forgotten about that PR!

martin-cs

Approve subject to @tautschnig 's comments about separate PRs.

Changing the C++ standard to C++ 17 on the cmake-based build-system.

Changing the C++ standard to C++ 17 on the gnu-make-based build-system

Changing the documentation COMPILING.md to reflect the change to C++17. Also increased the minimum G++ version to 7 as it is the lowest version with full C++17 support.

NlightNFotis · 2023-11-13T14:59:20Z

CMakeLists.txt

@@ -1,5 +1,9 @@
 cmake_minimum_required(VERSION 3.8)

+set(CMAKE_CXX_STANDARD 17)
+set(CMAKE_CXX_STANDARD_REQUIRED true)
+set(CMAKE_CXX_EXTENSIONS OFF)


Thank the stars we're not depending on any extensions from before (when I first saw this change I nearly had a heart attack, but then I noticed CI passing and I let out a sigh of relief).

esteffin requested a review from a team October 31, 2023 16:36

esteffin force-pushed the esteffin/cpp-17 branch from fe7dabe to 5088345 Compare October 31, 2023 16:36

NlightNFotis added the Version 6 Pull requests and issues requiring a major version bump label Oct 31, 2023

esteffin force-pushed the esteffin/cpp-17 branch from 1d4e388 to 12c5106 Compare October 31, 2023 17:10

tautschnig reviewed Nov 1, 2023

View reviewed changes

martin-cs approved these changes Nov 2, 2023

View reviewed changes

Enrico Steffinlongo added 4 commits November 6, 2023 13:39

Switch cmake build to c++17

9f2d209

Changing the C++ standard to C++ 17 on the cmake-based build-system.

Switch gnu make build to c++17

dff50eb

Changing the C++ standard to C++ 17 on the gnu-make-based build-system

Switch clang-format style to c++17

f3c7efe

Changed COMPILING.md to use c++17

032f68c

Changing the documentation COMPILING.md to reflect the change to C++17. Also increased the minimum G++ version to 7 as it is the lowest version with full C++17 support.

esteffin force-pushed the esteffin/cpp-17 branch from 12c5106 to 032f68c Compare November 6, 2023 13:39

This was referenced Nov 8, 2023

Use C++17 #6749

Closed

Build using C++17 #7172

Closed

NlightNFotis approved these changes Nov 13, 2023

View reviewed changes

NlightNFotis marked this pull request as ready for review November 13, 2023 15:00

NlightNFotis requested review from kroening and peterschrammel as code owners November 13, 2023 15:00

tautschnig approved these changes Nov 13, 2023

View reviewed changes

esteffin merged commit ea34799 into diffblue:develop Nov 13, 2023

esteffin deleted the esteffin/cpp-17 branch November 13, 2023 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change C++ version to C++17 #7989

Change C++ version to C++17 #7989

esteffin commented Oct 31, 2023

codecov bot commented Oct 31, 2023 •

edited

Loading

tautschnig commented Nov 1, 2023

tautschnig Nov 1, 2023

thomasspriggs Nov 1, 2023

esteffin Nov 1, 2023

tautschnig Nov 1, 2023

thomasspriggs Nov 2, 2023

tautschnig Nov 2, 2023

tautschnig Nov 2, 2023

esteffin Nov 3, 2023

martin-cs commented Nov 2, 2023

thomasspriggs commented Nov 2, 2023

martin-cs commented Nov 2, 2023

martin-cs left a comment

NlightNFotis Nov 13, 2023

Change C++ version to C++17 #7989

Change C++ version to C++17 #7989

Conversation

esteffin commented Oct 31, 2023

codecov bot commented Oct 31, 2023 • edited Loading

Codecov Report

tautschnig commented Nov 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-cs commented Nov 2, 2023

thomasspriggs commented Nov 2, 2023

martin-cs commented Nov 2, 2023

martin-cs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 31, 2023 •

edited

Loading