Skip to content

THRIFT-5660: lib: cpp: TTransportException: create thrift::numeric_cast - #2689

Merged
Jens-G merged 1 commit into
apache:masterfrom
cfriedt:use-std-cpp-instead-of-boost-numeric-cast
Nov 20, 2022
Merged

Jens-G merged 1 commit into
apache:masterfrom
cfriedt:use-std-cpp-instead-of-boost-numeric-cast

Conversation

@cfriedt

@cfriedt cfriedt commented Oct 1, 2022 •

Copy link
Copy Markdown
Contributor

This adds an equivalent implementation of boost::numeric_cast written purely in standard c++.

The implementation is relatively trivial and reduces the dependency on boost.

Adapted from
https://stackoverflow.com/a/49658950/5636218

  • Did you create an Apache Jira ticket? (not required for trivial changes)
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?

cfriedt pushed a commit to zephyrproject-rtos/gsoc-2022-thrift that referenced this pull request Oct 1, 2022
With this, we're starting to clean up the commit history a bit.
See #147  for details.

apache/thrift#2687
apache/thrift#2681
apache/thrift#2680
apache/thrift#2630
apache/thrift#2682
apache/thrift#2689

Signed-off-by: Christopher Friedt <cfriedt@meta.com>
cfriedt pushed a commit to zephyrproject-rtos/gsoc-2022-thrift that referenced this pull request Oct 2, 2022
With this, we're starting to clean up the commit history a bit.
See #147  for details.

apache/thrift#2687
apache/thrift#2681
apache/thrift#2680
apache/thrift#2630
apache/thrift#2682
apache/thrift#2689

Signed-off-by: Christopher Friedt <cfriedt@meta.com>
@cfriedt
cfriedt force-pushed the use-std-cpp-instead-of-boost-numeric-cast branch 2 times, most recently from 43f8761 to 547352c Compare October 23, 2022 12:34
@cfriedt cfriedt changed the title lib: cpp: TTransportException: create thrift::numeric_cast THRIFT-5660: lib: cpp: TTransportException: create thrift::numeric_cast Oct 25, 2022
@Jens-G Jens-G added the c++ Pull requests that update C++ code label Oct 25, 2022
@cfriedt

cfriedt commented Oct 26, 2022 •

Copy link
Copy Markdown
Contributor Author

Currently seeing a bunch of MSVC errors like the one below:

C:\projects\thrift\lib\cpp\src\thrift/numeric_cast.h(47): warning C4003: not enough arguments for function-like macro invocation 'max' [C:\projects\build\MSVC2017\x64\lib\cpp\thrift.vcxproj]
C:\projects\thrift\lib\cpp\src\thrift/numeric_cast.h(50): warning C4003: not enough arguments for function-like macro invocation 'max' [C:\projects\build\MSVC2017\x64\lib\cpp\thrift.vcxproj]
  ThreadManager.cpp
  TimerManager.cpp
  PeekProcessor.cpp
...
C:\projects\thrift\lib\cpp\src\thrift/numeric_cast.h(47): error C2589: '(': illegal token on right side of '::' 

It looks as though MSVC defines max(a, b) as a macro everywhere. This is ~questionable for C, but not good for C++ since we have std::max() but then also std::numeric_limits::max() and likely others.

This adds an equivalent implementation of `boost::numeric_cast`
written purely in standard c++.

The implementation is relatively trivial and reduces the
dependency on `boost`.

Adapted from
https://stackoverflow.com/a/49658950/5636218

Signed-off-by: Chris Friedt <cfriedt@meta.com>
@cfriedt
cfriedt force-pushed the use-std-cpp-instead-of-boost-numeric-cast branch from 547352c to 0fdd2b8 Compare October 26, 2022 12:43
@Jens-G
Jens-G merged commit 6e9cbbd into apache:master Nov 20, 2022
@cfriedt
cfriedt deleted the use-std-cpp-instead-of-boost-numeric-cast branch November 20, 2022 11:29
@Jens-G

Jens-G commented Nov 22, 2022

Copy link
Copy Markdown
Member

@cfriedt I guess this broke sth.
https://app.travis-ci.com/github/apache/thrift/jobs/589120686

[4/354] Building CXX object lib/cpp/CM...ansport/TNonblockingServerSocket.cpp.o
FAILED: lib/cpp/CMakeFiles/thriftnb.dir/src/thrift/transport/TNonblockingServerSocket.cpp.o
/usr/bin/g++ -DBOOST_ALL_DYN_LINK -DBOOST_TEST_DYN_LINK -DTHRIFT_STATIC_DEFINE -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/cpp -I../lib/cpp -I. -I../lib/cpp/src -O2 -g -DNDEBUG -std=c++11 -MD -MT lib/cpp/CMakeFiles/thriftnb.dir/src/thrift/transport/TNonblockingServerSocket.cpp.o -MF lib/cpp/CMakeFiles/thriftnb.dir/src/thrift/transport/TNonblockingServerSocket.cpp.o.d -o lib/cpp/CMakeFiles/thriftnb.dir/src/thrift/transport/TNonblockingServerSocket.cpp.o -c ../lib/cpp/src/thrift/transport/TNonblockingServerSocket.cpp
In file included from ../lib/cpp/src/thrift/transport/TTransport.h:25:0,
from ../lib/cpp/src/thrift/transport/TSocket.h:25,
from ../lib/cpp/src/thrift/transport/TNonblockingServerTransport.h:23,
from ../lib/cpp/src/thrift/transport/TNonblockingServerSocket.h:23,
from ../lib/cpp/src/thrift/transport/TNonblockingServerSocket.cpp:54:
../lib/cpp/src/thrift/transport/TTransportException.h:25:10: fatal error: thrift/numeric_cast.h: No such file or directory
#include <thrift/numeric_cast.h>
^~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
ninja: build stopped: subcommand failed.
The command "build/docker/run.sh" exited with 1.

@Jens-G

Jens-G commented Nov 22, 2022 •

Copy link
Copy Markdown
Member

We can fix it or revert it. Your decision.

BTW this also looks strange but don't know yet if it is related, may be caused by another patch.

https://app.travis-ci.com/github/apache/thrift/jobs/589120685

@cfriedt

cfriedt commented Nov 22, 2022

Copy link
Copy Markdown
Contributor Author

@cfriedt I guess this broke sth.
https://app.travis-ci.com/github/apache/thrift/jobs/589120686

That's weird - I'll take a look. Was that an autotools build or CMake build?

@Jens-G

Jens-G commented Nov 22, 2022

Copy link
Copy Markdown
Member

CMake I'd say

@cfriedt

cfriedt commented Nov 22, 2022

Copy link
Copy Markdown
Contributor Author

We can fix it or revert it. Your decision.

BTW this also looks strange but don't know yet if it is related, may be caused by another patch.

https://app.travis-ci.com/github/apache/thrift/jobs/589120685

I've seen this before and it's due to header reordering. I've been careful to avoid that FWIR.

There is that one header that defines THRIFT_IOCTL etc macros and because it does not itself include unistd, ioctl, fcntl, etc, it's ordering is fragile.

@cfriedt

cfriedt commented Nov 22, 2022

Copy link
Copy Markdown
Contributor Author

CMake I'd say

I see the issue. thrift/numeric_cast.h needs to be installed (of course). make-dist.sh tests builds from the distributed tarball.

Should be a quick fix.

@cfriedt

cfriedt commented Nov 22, 2022

Copy link
Copy Markdown
Contributor Author

We can fix it or revert it. Your decision.
BTW this also looks strange but don't know yet if it is related, may be caused by another patch.
https://app.travis-ci.com/github/apache/thrift/jobs/589120685

I've seen this before and it's due to header reordering. I've been careful to avoid that FWIR.

There is that one header that defines THRIFT_IOCTL etc macros and because it does not itself include unistd, ioctl, fcntl, etc, it's ordering is fragile.

Ah, this one is because <thrift/config.h> is not included

@cfriedt

cfriedt commented Nov 22, 2022

Copy link
Copy Markdown
Contributor Author

@Jens-G fixes are in #2733

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Pull requests that update C++ code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants