Added GRPC_USE_CPP_STD_LIB flag #20059
Conversation
veblush
requested review from
apolcyn,
AspirinSJL,
gnossen,
jtattermusch,
markdroth and
nicolasnoble
as code owners
veblush
added
release notes: no
veblush
force-pushed
the
cppflag
branch
2 times, most recently
from
39325c5
to
55553be
Compare
| // This is a workaround for the case where some compilers cannot build | ||
| // move-assignment of map with non-copyable but movable key. | ||
| // https://stackoverflow.com/questions/36475497/error-by-move-assignment-of-map-with-non-copyable-but-movable-key | ||
| swap(snapshot.dropped_requests, dropped_requests_); |
There was a problem hiding this comment.
Shouldn't this be std::swap()?
| @@ -315,7 +315,11 @@ grpc_slice XdsLrsRequestCreateAndEncode(const char* server_name) { | |||
| namespace { | |||
|
|
|||
| void LocalityStatsPopulate(envoy_api_v2_endpoint_UpstreamLocalityStats* output, | |||
| #if GRPC_USE_CPP_STD_LIB | |||
| Pair<const RefCountedPtr<XdsLocalityName>, | |||
There was a problem hiding this comment.
This smells like a bug in our existing grpc_core::Map<> implementation. Can we try to fix it such that it has the same API as std::map<>, so that this conditional is not required?
There was a problem hiding this comment.
Yeah I tried to change this to follow the standard but it seemed to require a significant amount of change so I didn't do that. Also it will be removed once C++ stdlib is successfully linked.
There was a problem hiding this comment.
It might be worth checking with @mhaidrygoog about the changes that would be needed in the current map implementation. He might be able to figure out a fairly quick way to do this.
Failing that, I'm fine with this conditional as a temporary thing, but please add a TODO indicating that it should go away once we've switched to always using the C++ standard library.
There was a problem hiding this comment.
I added the comment. My plan is removing all #else blocks of GRPC_USE_CPP_STD_LIB once C++ stdlib is fully enabled.
|
|
||
| #if GRPC_USE_CPP_STD_LIB | ||
|
|
||
| TEST(MapTest, Nop) {} |
There was a problem hiding this comment.
Shouldn't these tests pass regardless of whether we're using std::map<>?
There was a problem hiding this comment.
It doesn't work because test code depends on implementation details of Map such as Entry.
veblush
force-pushed
the
cppflag
branch
3 times, most recently
from
f37c836
to
7a28845
Compare
|
|
veblush
force-pushed
the
cppflag
branch
2 times, most recently
from
bae7925
to
7f2fa36
Compare
Added the global flag
GRPC_USE_CPP_STD_LIBenabling C++ standard library. When this is enabled,gpr::Mapis mapped tostd::mapinstead of in-house map.GRPC_ABSTRACTis replaced with= 0making legit pure virtual function.This flag is by default disabled. New test is added to "Bazel Basic build for C/C++" verifying grpc is built with the flag enabled.