Update C++ standard to 14 and clean up #19490

adamemerson · 2017-12-13T15:23:24Z

Still an improvement!

cbodley · 2017-12-13T15:31:37Z

src/common/backport_std.h

+#ifndef CEPH_COMMON_BACKPORT14_H
+#define CEPH_COMMON_BACKPORT14_H
+
+// Library code from C++14 that can be implemented in C++11.


nit: this comment can be updated

tchaikov · 2017-12-13T16:18:40Z

src/common/backport14.h

-using remove_reference_t = typename std::remove_reference<T>::type;
-template<typename T>
-using result_of_t = typename std::result_of<T>::type;
+// Template variable aliases from C++17 <type_traits>


can we guard this with #if __cplusplus < 201703L, and just put these templates into namespace std?

adamemerson · 2017-12-13T16:22:17Z

@tchaikov Maaaaybe? It's Undefined Behavior and Bad and Naughty and so I'd rather not, but it would probably work.

I have a /better/ solution for this sort of problem I've been working on from time to time.

Generally, if we put everything in core inside the ceph namespace

And encapsulated things like this in a few inline namespaces so we could easily pull them into RGW and RBD and whatnot

Then we could easily change definitions to usings and things like that, but it's a fairly large project and I haven't had a chance to work on it recently.

cbodley · 2017-12-13T16:40:31Z

It's Undefined Behavior and Bad and Naughty and so I'd rather not

that's my feeling as well. it might make our transitions between c++ versions a bit easier, but i don't think we do that often enough to get so adventurous

adamemerson · 2017-12-13T16:52:54Z

Yeah, once we go to 17, whenever the CentOS people get their thing done, we won't have to change this sort of thing until …2020?

tchaikov

@adamemerson @cbodley makes sense to me!

Fix a couple problems. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

adamemerson · 2017-12-14T03:40:27Z

@tchaikov One minor error crept in during a rebase, just fixed it.

adamemerson · 2017-12-14T03:40:47Z

Well, major in that it broke everything, but minor in terms of scope and fix and complexity.

tchaikov · 2017-12-14T03:54:55Z

src/common/backport14.h

-template<typename T>
-using remove_reference_t = typename std::remove_reference<T>::type;
-template<typename T>
-using result_of_t = typename std::result_of<T>::type;


In file included from /home/jenkins-build/build/workspace/ceph-pull-requests/src/common/dout.h:22:0, from /home/jenkins-build/build/workspace/ceph-pull-requests/src/common/debug.h:18, from /home/jenkins-build/build/workspace/ceph-pull-requests/src/compressor/Compressor.cc:22: /home/jenkins-build/build/workspace/ceph-pull-requests/src/common/config.h:171:11: error: 'result_of_t' in namespace 'ceph' does not name a template type ceph::result_of_t<Callback(const T&, Args...)> { ^~~~~~~~~~~ /home/jenkins-build/build/workspace/ceph-pull-requests/src/common/config.h:171:22: error: expected initializer before '<' token ceph::result_of_t<Callback(const T&, Args...)> { ^ src/compressor/CMakeFiles/compressor_objs.dir/build.make:62: recipe for target 'src/compressor/CMakeFiles/compressor_objs.dir/Compressor.cc.o' failed

@adamemerson

tchaikov · 2017-12-14T03:55:38Z

src/test/common/test_backport14.cc

@@ -14,7 +14,7 @@
 *
 */

-#include "common/backport14.h" // include first: tests that header is standalone


@adamemerson we need to rename this source file. see https://github.com/ceph/ceph/pull/19490/files#diff-a67cc84c25b8865adfc63fed42d6ceffR268

C++14 provides the ability to define template variables, which the C++17 library puts to good use. We define those same aliases. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

adamemerson · 2017-12-14T06:40:14Z

Oops. My own merged patch did me in. Fixed and pushed. Will run a local compile to check anything else.

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

Since we're on C++14, we're just backporting from future standards generally. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

adamemerson · 2017-12-14T17:32:19Z

@tchaikov All right, got the problems I introduced by rebasing cleared away.

tchaikov · 2017-12-16T04:30:18Z

cbodley · 2017-12-18T14:16:45Z

exciting! 🎉 you are my heroes, @tchaikov and @adamemerson!

chardan · 2017-12-18T19:11:13Z

Very cool!!

theanalyst · 2017-12-28T08:44:37Z

Awesome!

smithfarm · 2019-09-02T14:22:23Z

@adamemerson @chardan Could you help me diagnose the build failure we are seeing in nautilus? Here are some instances of it:

#30040
#30049
#30050

It blurts out a gazillion errors, but the first one is:

/home/jenkins-build/build/workspace/ceph-pull-requests/src/common/convenience.h:46:24: error: 'result_of_t' is not a member of 'std'
   boost::optional<std::result_of_t<F(const std::decay_t<T>)>>

which would appear to be related to this PR?

Also, at https://en.cppreference.com/w/cpp/types/result_of I saw that this feature added by C++14 is now deprecated and due to be removed in C++20... not sure what should be done (if anything) about that.

smithfarm · 2019-09-02T14:34:44Z

@tchaikov Looks like "make check" is broken in nautilus? (See the previous comment - all of the "make check" failures in nautilus seem to be caused by this same issue, i.e. ostensibly "'result_of_t' is not a member of 'std'"...)

tchaikov · 2019-09-02T14:59:42Z

@smithfarm thanks for looking into this. yeah, i am afraid "rëtest this please" won't work this time. will investigate this early tomorrow.

chardan · 2019-09-02T18:44:37Z

@smithfarm Yeah, for C++17 result_of was deprecated, and removed in C++20 (see the "resolved issues" section here: https://tinyurl.com/km5tkhu). Chances are good that you can just replace it with "std::invoke_result" and "std::invoke_result_t".

chardan · 2019-09-02T18:57:02Z

I also saw the earlier part of the conversation related to feature testing, and wanted to point out that there are lots of new feature-test macros made available:
https://en.cppreference.com/w/cpp/feature_test
...and a good article from Study Group 10 on using them:
https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations

tchaikov · 2019-09-03T05:37:40Z

@smithfarm i failed to reproduce the FTBFS in my own xenial container and in #30083 . the only thing i can think of is to use GCC-8. see #30089.

but i think the root cause is still unknown. as the error message put

/home/jenkins-build/build/workspace/ceph-pull-requests/src/common/ceph_time.h:504:16: warning: variable templates only available with -std=c++14 or -std=gnu++14
 constexpr bool converts_to_timespec_v = converts_to_timespec<Clock>::value;
                ^~~~~~~~~~~~~~~~~~~~~~

and we do pass -std=c++17 to g++. see

ceph/src/CMakeLists.txt

Line 115 in e1f6710

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++17")

cbodley · 2019-09-03T14:02:58Z

in the VERBOSE output, i see that an extra -std=gnu++11 is getting sprinkled into some targets after the -std=c++17. is it possible that cmake is getting confused by the set(CMAKE_CXX_STANDARD 11)s in src/dmclock/CMakeLists.txt and src/fmt/support/cmake/cxx14.cmake? on master, we have set(CMAKE_CXX_STANDARD 17), but here we're just adding -std=c++17 to CMAKE_CXX_FLAGS

tchaikov · 2019-09-03T14:11:12Z

@cbodley thanks a lot! that explains. i ran into the same issue couple weeks ago before switching over to cmake 3.10.

i believe that's the root cause.

tchaikov · 2019-09-03T14:17:58Z

but we need to either use cmake 3.10 for addressing this issue or hack libfmt to prevent it from falling back to C++11.

cbodley · 2019-09-03T14:35:43Z

what do we need from cmake 3.10? is that what adds 17 as an option for CXX_STANDARD?

tchaikov · 2019-09-03T14:40:07Z

yeah, 3.8 introduced the support of CXX_STANDARD=17.

tchaikov · 2019-09-03T14:49:28Z

i am working on a fix for libfmt and dmclock.

tchaikov · 2019-09-03T17:13:48Z

#30113 #30114

adamemerson requested a review from tchaikov December 13, 2017 15:23

adamemerson mentioned this pull request Dec 13, 2017

common: Just a few minor things to followup from using GCC 7 #19403

Closed

cbodley reviewed Dec 13, 2017

View reviewed changes

tchaikov reviewed Dec 13, 2017

View reviewed changes

adamemerson force-pushed the wip-hypomodern branch from 4730bcc to 73d692d Compare December 13, 2017 16:19

tchaikov approved these changes Dec 14, 2017

View reviewed changes

tchaikov added build/ops cleanup common needs-qa wip-kefu-testing labels Dec 14, 2017

build: Uplevel to C++14

70da1ee

Fix a couple problems. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

adamemerson force-pushed the wip-hypomodern branch from 73d692d to dd927b8 Compare December 14, 2017 03:40

tchaikov requested changes Dec 14, 2017

View reviewed changes

tchaikov removed needs-qa wip-kefu-testing labels Dec 14, 2017

common: Swap C++14 template type aliases for variables

bbe0aff

C++14 provides the ability to define template variables, which the C++17 library puts to good use. We define those same aliases. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

adamemerson force-pushed the wip-hypomodern branch from dd927b8 to c29d42b Compare December 14, 2017 06:39

adamemerson added 3 commits December 14, 2017 11:40

common: Remove backported max and make_unique

b280bf4

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

common: Get rid of artifact filename

2d45f80

Since we're on C++14, we're just backporting from future standards generally. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

common: Switch from boost::shared_mutex to std::shared_mutex

e08a1cd

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

adamemerson force-pushed the wip-hypomodern branch from c29d42b to e08a1cd Compare December 14, 2017 16:40

adamemerson added the needs-qa label Dec 14, 2017

tchaikov approved these changes Dec 15, 2017

View reviewed changes

tchaikov added the wip-kefu-testing label Dec 15, 2017

tchaikov merged commit 419a182 into ceph:master Dec 16, 2017

adamemerson deleted the wip-hypomodern branch December 20, 2017 23:03

smithfarm mentioned this pull request Sep 2, 2019

nautilus: core: mon: C_AckMarkedDown has not handled the Callback Arguments #29997

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update C++ standard to 14 and clean up #19490

Update C++ standard to 14 and clean up #19490

adamemerson commented Dec 13, 2017

cbodley Dec 13, 2017

tchaikov Dec 13, 2017

adamemerson commented Dec 13, 2017

cbodley commented Dec 13, 2017

adamemerson commented Dec 13, 2017

tchaikov left a comment

adamemerson commented Dec 14, 2017

adamemerson commented Dec 14, 2017

tchaikov Dec 14, 2017

tchaikov Dec 14, 2017

adamemerson commented Dec 14, 2017

adamemerson commented Dec 14, 2017

tchaikov commented Dec 16, 2017

cbodley commented Dec 18, 2017

chardan commented Dec 18, 2017

theanalyst commented Dec 28, 2017

smithfarm commented Sep 2, 2019 •

edited

smithfarm commented Sep 2, 2019 •

edited

tchaikov commented Sep 2, 2019

chardan commented Sep 2, 2019

chardan commented Sep 2, 2019

tchaikov commented Sep 3, 2019

cbodley commented Sep 3, 2019

tchaikov commented Sep 3, 2019

tchaikov commented Sep 3, 2019

cbodley commented Sep 3, 2019

tchaikov commented Sep 3, 2019

tchaikov commented Sep 3, 2019

tchaikov commented Sep 3, 2019

Update C++ standard to 14 and clean up #19490

Update C++ standard to 14 and clean up #19490

Conversation

adamemerson commented Dec 13, 2017

cbodley Dec 13, 2017

Choose a reason for hiding this comment

tchaikov Dec 13, 2017

Choose a reason for hiding this comment

adamemerson commented Dec 13, 2017

cbodley commented Dec 13, 2017

adamemerson commented Dec 13, 2017

tchaikov left a comment

Choose a reason for hiding this comment

adamemerson commented Dec 14, 2017

adamemerson commented Dec 14, 2017

tchaikov Dec 14, 2017

Choose a reason for hiding this comment

tchaikov Dec 14, 2017

Choose a reason for hiding this comment

adamemerson commented Dec 14, 2017

adamemerson commented Dec 14, 2017

tchaikov commented Dec 16, 2017

cbodley commented Dec 18, 2017

chardan commented Dec 18, 2017

theanalyst commented Dec 28, 2017

smithfarm commented Sep 2, 2019 • edited

smithfarm commented Sep 2, 2019 • edited

tchaikov commented Sep 2, 2019

chardan commented Sep 2, 2019

chardan commented Sep 2, 2019

tchaikov commented Sep 3, 2019

cbodley commented Sep 3, 2019

tchaikov commented Sep 3, 2019

tchaikov commented Sep 3, 2019

cbodley commented Sep 3, 2019

tchaikov commented Sep 3, 2019

tchaikov commented Sep 3, 2019

tchaikov commented Sep 3, 2019

smithfarm commented Sep 2, 2019 •

edited

smithfarm commented Sep 2, 2019 •

edited