Fix compilation with GCC9 and MSVC 2019 with CMAKE_CXX_STANDARD=20 #6648

syoliver · 2020-04-05T22:08:05Z

Add the ability to build with different std version though -DCMAKE_CXX_STANDARD=11/14/17/20 on the cmake command line
Implicit capture of this via [=] deprecated in C++20
Implicity copy operator deprecated in gcc 9
std::random_shuffle deprecated in C++17 and removed in C++20
Minimal rebuild flag of MSVC is deprectaed and is forbidden with /std:c++latest (C++20)
Added MSVC 2019 C++11 & MSVC 2019 C++20 in AppVeyor
Added GCC 9 C++11 & GCC9 C++30 in Travis

Any interest ?

…biden with /std:c++latest

adamretter · 2020-04-06T09:48:04Z

@syoliver Are these changes still compatible with VS 2015?

syoliver · 2020-04-06T12:00:48Z

@syoliver Are these changes still compatible with VS 2015?

According to appveyor it should be compatible, but I admit I didn't test myself with VS 2015. I should try the /Gm with 2015 before publishing the PR, but according to the documentation it should have been deprecated long time ago.

The default is always -std=c++11, I just added a way to compile with C++20 in the cmake.

The implicit capture of this is tested in the makefile/cmakelists because depending on the compiler, [=] and [=, this] can compile or not.

The only issue I have is with clang XCode makefile build, I don't find why it fails :-(

adamretter · 2020-04-06T12:06:30Z

@syoliver As long as AppVeyor for VS2015 passes then I am a happy person :-)

(Add logs...)

syoliver · 2020-04-07T08:15:54Z

Timeout reached in travis, but it seems to work.
Is it interesting ? Any comment ?

@adamretter : Visual Studio 2015 doesn't have warning on /Gm, then I leaved this option for 2015. 2017 and 2019 have warning, 2019 with /std:c++latest has error, then I removed this option for 2017 and more.

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-04-10T18:16:51Z

@syoliver has updated the pull request. Re-import the pull request

facebook-github-bot · 2020-04-10T18:51:00Z

@syoliver has updated the pull request. Re-import the pull request

pdillinger

Thanks, LGTM

pdillinger · 2020-04-10T18:33:58Z

db/db_bloom_filter_test.cc

@@ -1343,8 +1344,9 @@ TEST_F(DBBloomFilterTest, OptimizeFiltersForHits) {
  for (int i = 0; i < numkeys; i += 2) {
    keys.push_back(i);
  }
-  std::random_shuffle(std::begin(keys), std::end(keys));
-
+  std::random_device rng;


I think we should unify all of these, including

test_util/transaction_test_util.cc: std::shuffle(set_vec.begin(), set_vec.end(), std::random_device{});

with a single implementation in util/random.h,cc

Though we can do this after merging this PR.

@pdillinger Yes you are right, I would be happy to propose something after this PR if it's good for you. Else I can try here, but I would prefer making it in another step.

pdillinger · 2020-04-10T18:44:32Z

memory/concurrent_arena.h

@@ -49,7 +49,9 @@ class ConcurrentArena : public Allocator {

  char* Allocate(size_t bytes) override {
    return AllocateImpl(bytes, false /*force_arena*/,
-                        [=]() { return arena_.Allocate(bytes); });
+      [ROCKSDB_THIS_LAMBDA_CAPTURE]() {


Unfortunate, but I guess we need it to avoid newer C++ and older C++ disagreeing on which is worth a warning

@pdillinger Honestly, it we can discusse about it. I have another way to fix it :

Use the [=, this] everywhere, which is the new way to handle such pattern

Ignore warning on pre std 20 compilers

At this time, we write C++11 with warnings as errors. We aren't going to use a patchwork of trying to ignore this specific warning for various compilers in our default configuration. (I haven't even found a way to turn it off in gcc while keeping -Werror.)

Exactly which C++20 compiler are you getting an error with? I tried latest gcc, clang and msvc on godbolt.org and was not able to get even a warning. Can't you suppress in the C++20 case? (deprecated != removed)

https://godbolt.org/z/8NCWeL
https://godbolt.org/z/2eygL_
https://godbolt.org/z/gPKt3s (see also https://developercommunity.visualstudio.com/content/problem/749985/msvc-1922-emitts-c4626-warning-on-simple-lambdas-w.html)

@pdillinger
With gcc 9 -std=c++11: https://godbolt.org/z/Z2DrYm
With gcc 9 -std=c++2a: https://godbolt.org/z/knfpAh

With "-Wno-error=deprecated" it may fix the error and leave the warning, but I don't like it...

On the other hand, I think there is a better (and easier) way : just dop the default capture and capture explicitly. It is cleaner and should work everywhere.

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

pdillinger

I'm going to tweak this because it broke FB's buck integration

pdillinger · 2020-04-14T17:23:28Z

Superseded by #6697, which extends this one. Thanks for the valuable contribution!

syoliver · 2020-04-14T17:55:51Z

Superseded by #6697, which extends this one. Thanks for the valuable contribution!
Thank you @pdillinger great job :-)

Summary: Based on #6648 (CLA Signed), but heavily modified / extended: * Implicit capture of this via [=] deprecated in C++20, and [=,this] not standard before C++20 -> now using explicit capture lists * Implicit copy operator deprecated in gcc 9 -> add explicit '= default' definition * std::random_shuffle deprecated in C++17 and removed in C++20 -> migrated to a replacement in RocksDB random.h API * Add the ability to build with different std version though -DCMAKE_CXX_STANDARD=11/14/17/20 on the cmake command line * Minimal rebuild flag of MSVC is deprecated and is forbidden with /std:c++latest (C++20) * Added MSVC 2019 C++11 & MSVC 2019 C++20 in AppVeyor * Added GCC 9 C++11 & GCC9 C++20 in Travis Pull Request resolved: #6697 Test Plan: make check and CI Reviewed By: cheng-chang Differential Revision: D21020318 Pulled By: pdillinger fbshipit-source-id: 12311be5dbd8675a0e2c817f7ec50fa11c18ab91

Summary: Based on facebook/rocksdb#6648 (CLA Signed), but heavily modified / extended: * Implicit capture of this via [=] deprecated in C++20, and [=,this] not standard before C++20 -> now using explicit capture lists * Implicit copy operator deprecated in gcc 9 -> add explicit '= default' definition * std::random_shuffle deprecated in C++17 and removed in C++20 -> migrated to a replacement in RocksDB random.h API * Add the ability to build with different std version though -DCMAKE_CXX_STANDARD=11/14/17/20 on the cmake command line * Minimal rebuild flag of MSVC is deprecated and is forbidden with /std:c++latest (C++20) * Added MSVC 2019 C++11 & MSVC 2019 C++20 in AppVeyor * Added GCC 9 C++11 & GCC9 C++20 in Travis Pull Request resolved: facebook/rocksdb#6697 Test Plan: make check and CI Reviewed By: cheng-chang Differential Revision: D21020318 Pulled By: pdillinger fbshipit-source-id: 12311be5dbd8675a0e2c817f7ec50fa11c18ab91 Signed-off-by: Changlong Chen <levisonchen@live.cn>

syoliver-se added 6 commits April 5, 2020 21:30

Fix implicit capture of ‘this’ via ‘[=]’ is deprecated in C++20

af91b57

Allow to choose C++ version in cmake command line

880549c

Fix deprecated copy warning in gcc 9

4a1df6e

Minimal Rebuild is deprecated in visual studio since 2015 and is forb…

2756589

…biden with /std:c++latest

random_shuffle is deprecated in C++14 and removed in C++17

cea96d0

Fix implicit this lambda capture deprecation

4e46260

facebook-github-bot added the CLA Signed label Apr 5, 2020

Add implicit lambda capture in raw makefile

96f7359

(Add logs...)

adamretter added the windows label Apr 6, 2020

syoliver-se added 2 commits April 6, 2020 21:07

Add MSVC2019 to AppVeyor

5962870

Add gcc-9 with c++11 and c++20 to travis

d31e395

syoliver marked this pull request as ready for review April 7, 2020 08:15

facebook-github-bot reviewed Apr 10, 2020

View reviewed changes

ghost requested a review from pdillinger April 10, 2020 17:34

pdillinger added 2 commits April 10, 2020 11:06

Merge remote-tracking branch 'origin/master' into pr/6648

43eee91

Reduce cmake variants built on each PR

87f3151

Fix whitespace / noeol in CMakeLists.txt

831fb29

pdillinger approved these changes Apr 10, 2020

View reviewed changes

facebook-github-bot reviewed Apr 10, 2020

View reviewed changes

pdillinger requested changes Apr 10, 2020

View reviewed changes

pdillinger mentioned this pull request Apr 14, 2020

C++20 compatibility #6697

Closed

pdillinger closed this Apr 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compilation with GCC9 and MSVC 2019 with CMAKE_CXX_STANDARD=20 #6648

Fix compilation with GCC9 and MSVC 2019 with CMAKE_CXX_STANDARD=20 #6648

syoliver commented Apr 5, 2020 •

edited

Loading

adamretter commented Apr 6, 2020

syoliver commented Apr 6, 2020

adamretter commented Apr 6, 2020

syoliver commented Apr 7, 2020 •

edited

Loading

facebook-github-bot left a comment

facebook-github-bot commented Apr 10, 2020

facebook-github-bot commented Apr 10, 2020

pdillinger left a comment

pdillinger Apr 10, 2020

syoliver Apr 11, 2020

pdillinger Apr 10, 2020

syoliver Apr 11, 2020

pdillinger Apr 13, 2020 •

edited

Loading

syoliver Apr 14, 2020

facebook-github-bot left a comment

pdillinger left a comment

pdillinger commented Apr 14, 2020

syoliver commented Apr 14, 2020

Fix compilation with GCC9 and MSVC 2019 with CMAKE_CXX_STANDARD=20 #6648

Fix compilation with GCC9 and MSVC 2019 with CMAKE_CXX_STANDARD=20 #6648

Conversation

syoliver commented Apr 5, 2020 • edited Loading

adamretter commented Apr 6, 2020

syoliver commented Apr 6, 2020

adamretter commented Apr 6, 2020

syoliver commented Apr 7, 2020 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 10, 2020

facebook-github-bot commented Apr 10, 2020

pdillinger left a comment

Choose a reason for hiding this comment

pdillinger Apr 10, 2020

Choose a reason for hiding this comment

syoliver Apr 11, 2020

Choose a reason for hiding this comment

pdillinger Apr 10, 2020

Choose a reason for hiding this comment

syoliver Apr 11, 2020

Choose a reason for hiding this comment

pdillinger Apr 13, 2020 • edited Loading

Choose a reason for hiding this comment

syoliver Apr 14, 2020

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

pdillinger left a comment

Choose a reason for hiding this comment

pdillinger commented Apr 14, 2020

syoliver commented Apr 14, 2020

syoliver commented Apr 5, 2020 •

edited

Loading

syoliver commented Apr 7, 2020 •

edited

Loading

pdillinger Apr 13, 2020 •

edited

Loading