THRIFT-4720:migrate code to c++11 #1677

cyyever · 2019-01-05T02:09:56Z

As discussed in THRIFT-4678.

This closes #1448

jeking3

Need autoconf build changes.
Need Travis CI changes (I believe there is a C++03 job somewhere in there that can be removed).
Need to make sure if someone specifies C++03 in cmake or autoconf that it fails early.
THRIFT-4441 made most of boost configurable, maybe look at starting there and removing the backwards compatibility part of it, and finishing it up? See Thrift-4441: Support compilation without Boost #1448

jeking3 · 2019-01-05T02:26:35Z

build/cmake/DefinePlatformSpecifc.cmake

@@ -97,21 +97,14 @@ elseif(WITH_STDTHREADS)
 endif()

 # C++ Language Level
-set(CXX_LANGUAGE_LEVEL "C++${CMAKE_CXX_STANDARD}")


I believe CXX_LANGUAGE_LEVEL is used in the build summary output which is why it was being built as a string.

build/cmake/DefinePlatformSpecifc.cmake

jeking3 · 2019-01-05T06:16:59Z

lib/cpp/CMakeLists.txt

-    )
-    list(APPEND SYSLIBS "${Boost_LIBRARIES}")
-elseif(UNIX AND NOT WITH_STDTHREADS)
+if(UNIX AND NOT WITH_STDTHREADS)


We don't need pthread raw support any more. Only use std::threads, get rid of all the options? Perhaps in the next round?

yes，next round

jeking3 · 2019-01-05T06:18:13Z

lib/cpp/Makefile.am

@@ -106,15 +106,9 @@ libthrift_la_SOURCES = src/thrift/TApplicationException.cpp \
                       src/thrift/server/TThreadPoolServer.cpp \
                       src/thrift/server/TThreadedServer.cpp

-if WITH_BOOSTTHREADS


Hmm, where is the STDTHREADS?

jeking3 · 2019-01-05T06:19:45Z

lib/cpp/src/thrift/concurrency/PlatformThreadFactory.h

-#if USE_BOOST_THREAD
-#  include <thrift/concurrency/BoostThreadFactory.h>
-#elif USE_STD_THREAD
+#if USE_STD_THREAD


I'd really recommend getting rid of PosixThreadFactory.h and move the contents of StdThreadFactory.h into PlatformThreadFactory.h, and maybe rename it to TThreadFactory using std::thread only.

I'd really recommend getting rid of PosixThreadFactory.h and move the contents of StdThreadFactory.h into PlatformThreadFactory.h, and maybe rename it to TThreadFactory using std::thread only.

Posixthread has policy and priority settings which are lack in std::thread,So deleting it may break many existing codes,and these settings may be important for performance.

Tweaking thread priority is usually a slippery slope... and the pthread code has platform incompatibilities. We should remove it all if we're going to do this. :)

jeking3 · 2019-01-05T06:24:24Z

Definitely getting there; would be nice to eliminate the stdcxx namespace and header (or keep it and just keep the C++11 stuff for now), eliminate the posix and platform thread factory gunk, and just have one TThreadFactory which is the std::thread code.

cyyever · 2019-01-05T08:45:57Z

Definitely getting there; would be nice to eliminate the stdcxx namespace and header (or keep it and just keep the C++11 stuff for now), eliminate the posix and platform thread factory gunk, and just have one TThreadFactory which is the std::thread code.

stdcxx has been removed.

lib/cpp/src/thrift/server/TServer.h

jeking3 · 2019-01-06T16:14:33Z

Turned into quite the pull request. Are you done making changes? I'd like to see PosixThreadFactory and PlatformThreadFactory gone. The whole concept was not done well (it didn't even use inheritance!). Are you planning to do that after?

cyyever · 2019-01-07T02:01:39Z

I will give a separated PR for removing pthread later

jeking3 · 2019-01-07T02:39:53Z

These changes will break backwards compatibility, so all of these changes will need to be documented as breaking in the lib/cpp/README.md, in the top level CHANGES. The code will be cleaner and standards compliant in the end, which I think is a good thing. Folks can continue to use 0.12.0 and earlier to remain compatible with the code they have now.

emmenlau · 2019-01-07T10:18:47Z

I just chip in to say a bit "thank you"! I'm looking forward to a more C++-1-based codebase. Two thumbs up!

jeking3 · 2019-01-07T13:11:21Z

I'll follow this up with a change to the cpp readme, top level changes, and top level languages files.

jeking3 · 2019-01-07T15:04:39Z

Darnit. I meant to squash this into one commit, but I rebased it instead! grumble

jeking3 · 2019-01-09T15:40:15Z

While this removed the runtime dynamic library dependencies on boost, it did not remove the requirement to have boost headers present to build the runtime library. While boost is required to run the tests, it shouldn't be required to just build the CPP library. There are about 40 more mentions of boost in the lib/cpp/src headers that need to be converted to std concepts, if they exist; or in the case of boost::noncopyable, do something else. Note that the pull request for THRIFT-4441 had a solution for this.

since thrift has dropped support for stdcxx.h that makes jaeger incompatible with latest thrift and hence jaeger failure: /home/ideepika/ceph-primary/ceph/src/jaegertracing/jaeger-client-cpp/src/jaegertracing/thrift-gen/agent_types.h:18:10: fatal error: thrift/stdcxx.h: No such file or directory 18 | #include <thrift/stdcxx.h> | ^~~~~~~~~~~~~~~~~ this change will make jaeger compatible with latest thrift. see: apache/thrift#1677 Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>

helmesjo · 2020-11-05T12:54:22Z

@jeking3
I'm a bit torn with what is required... So boost is only required when building the thrift library, not when linking it (runtime)? I'm building my conan-package. If I use boost only for building, then my simple test_package.cpp fails with the following (among others):

TWinsockSingleton.h(34,10): fatal error C1083: Cannot open include file: 'boost/noncopyable.hpp': No such file or directory [...]

So do I understand your above comment correct that there is no need to link boost, but some headers are still required when building my code that includes the generated thrift-stubs?

You mention THRIFT-4441 but that has already been closed and is in 0.13 (allegedly), but obviously I still see the above issue (and boost is to some extent still required to both build and consume thrift 0.13).

So basically:
What is the state regarding boost? Is there anything in the works to get rid of boost completely? Any light shed on this would be greatly appreciated!

* thrift: drop stdcxx namespace since thrift has dropped support for stdcxx.h that makes jaeger incompatible with latest thrift and hence jaeger failure: /home/ideepika/ceph-primary/ceph/src/jaegertracing/jaeger-client-cpp/src/jaegertracing/thrift-gen/agent_types.h:18:10: fatal error: thrift/stdcxx.h: No such file or directory 18 | #include <thrift/stdcxx.h> | ^~~~~~~~~~~~~~~~~ this change will make jaeger compatible with latest thrift. see: apache/thrift#1677 Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com> * thrift: fix warning due to difference in naming. fixes warning: CMake Warning (dev) at /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:272 (message): The package name passed to `find_package_handle_standard_args` (THRIFT) does not match the name of the calling package (thrift). This can lead to problems in calling code that expects `find_package` result variables (e.g., `_FOUND`) to follow a certain pattern. Call Stack (most recent call first): cmake/Findthrift.cmake:92 (FIND_PACKAGE_HANDLE_STANDARD_ARGS) CMakeLists.txt:63 (find_package) This warning is for project developers. Use -Wno-dev to suppress it. Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com> Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

root and others added 3 commits January 5, 2019 10:03

force c++11 in cmake

7dd4513

use noexcept instead of throw() in library

6a46fdd

use noexcept instead of throw() in compiler generated code

ee9cb47

cyyever changed the title ~~migrate code to c++11~~ THRIFT-4720:migrate code to c++11 Jan 5, 2019

jeking3 requested changes Jan 5, 2019

View reviewed changes

cyyever added 5 commits January 5, 2019 10:52

force c++11 in GNU autotools

3aee052

allow specifing CMAKE_CXX_STANDARD above c++11

3a7f472

remove boost::thread and boost::mutex code

7fba2ea

remove boost::thread in CI

fddb469

remove boost::thread in CI

ef84f70

jeking3 reviewed Jan 5, 2019

View reviewed changes

cyyever added 4 commits January 5, 2019 14:49

remove boost smart ptr

631e02c

remove std tr1

b7afdec

remove stdcxx namespace and use std directly

4218f7a

remove stdcxx namespace and use std directly

1fd21e5

cyyever added 10 commits January 5, 2019 17:09

remove stdcxx namespace and use std directly

478b7df

revert noexcept changings

bb65fc6

remove std tr1

f326a38

replace boost::atomic with std::atomic

b94a7ab

replace boost static_assert with static_assert

034319a

replace boost unique_ptr with std::unique_ptr

a7c9f33

add static_assert message

d878ffc

set c++11 in proper place

7a80284

remove unused boost headers

e1b8760

remove unused variables

2a6c5b0

jeking3 reviewed Jan 6, 2019

View reviewed changes

lib/cpp/src/thrift/server/TServer.h Show resolved Hide resolved

jeking3 approved these changes Jan 7, 2019

View reviewed changes

jeking3 merged commit 0140cbf into apache:master Jan 7, 2019

ideepika mentioned this pull request Aug 21, 2020

[thrift]: drop stdcxx namespace jaegertracing/jaeger-client-cpp#239

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

THRIFT-4720:migrate code to c++11 #1677

THRIFT-4720:migrate code to c++11 #1677

cyyever commented Jan 5, 2019 •

edited by jeking3

Loading

jeking3 left a comment •

edited

Loading

jeking3 Jan 5, 2019

jeking3 Jan 5, 2019

cyyever Jan 5, 2019

jeking3 Jan 5, 2019

jeking3 Jan 5, 2019

cyyever Jan 5, 2019 •

edited

Loading

jeking3 Jan 6, 2019

jeking3 commented Jan 5, 2019

cyyever commented Jan 5, 2019

jeking3 commented Jan 6, 2019

cyyever commented Jan 7, 2019

jeking3 commented Jan 7, 2019

emmenlau commented Jan 7, 2019

jeking3 commented Jan 7, 2019

jeking3 commented Jan 7, 2019

jeking3 commented Jan 9, 2019

helmesjo commented Nov 5, 2020 •

edited

Loading

THRIFT-4720:migrate code to c++11 #1677

THRIFT-4720:migrate code to c++11 #1677

Conversation

cyyever commented Jan 5, 2019 • edited by jeking3 Loading

jeking3 left a comment • edited Loading

Choose a reason for hiding this comment

jeking3 Jan 5, 2019

Choose a reason for hiding this comment

jeking3 Jan 5, 2019

Choose a reason for hiding this comment

cyyever Jan 5, 2019

Choose a reason for hiding this comment

jeking3 Jan 5, 2019

Choose a reason for hiding this comment

jeking3 Jan 5, 2019

Choose a reason for hiding this comment

cyyever Jan 5, 2019 • edited Loading

Choose a reason for hiding this comment

jeking3 Jan 6, 2019

Choose a reason for hiding this comment

jeking3 commented Jan 5, 2019

cyyever commented Jan 5, 2019

jeking3 commented Jan 6, 2019

cyyever commented Jan 7, 2019

jeking3 commented Jan 7, 2019

emmenlau commented Jan 7, 2019

jeking3 commented Jan 7, 2019

jeking3 commented Jan 7, 2019

jeking3 commented Jan 9, 2019

helmesjo commented Nov 5, 2020 • edited Loading

cyyever commented Jan 5, 2019 •

edited by jeking3

Loading

jeking3 left a comment •

edited

Loading

cyyever Jan 5, 2019 •

edited

Loading

helmesjo commented Nov 5, 2020 •

edited

Loading