Copilot commented on code in PR #3346:
URL: https://github.com/apache/brpc/pull/3346#discussion_r3417809521
##########
test/CMakeLists.txt:
##########
@@ -53,8 +53,8 @@ else()
endif()
set(CMAKE_CPP_FLAGS "${CMAKE_CPP_FLAGS} ${DEFINE_CLOCK_GETTIME}
-DBRPC_WITH_GLOG=${WITH_GLOG_VAL} -DBRPC_WITH_RDMA=${WITH_RDMA_VAL}")
-set(CMAKE_CPP_FLAGS "${CMAKE_CPP_FLAGS} -DBTHREAD_USE_FAST_PTHREAD_MUTEX
-D__const__=__unused__ -D_GNU_SOURCE -DUSE_SYMBOLIZE -DNO_TCMALLOC
-D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS
-DUNIT_TEST -Dprivate=public -Dprotected=public
-DBVAR_NOT_LINK_DEFAULT_VARIABLES -D__STRICT_ANSI__ -include
${PROJECT_SOURCE_DIR}/test/sstream_workaround.h")
-set(CMAKE_CXX_FLAGS "${CMAKE_CPP_FLAGS} -g -O2 -pipe -Wall -W -fPIC
-fstrict-aliasing -Wno-invalid-offsetof -Wno-unused-parameter
-fno-omit-frame-pointer")
+set(CMAKE_CPP_FLAGS "${CMAKE_CPP_FLAGS} -DBTHREAD_USE_FAST_PTHREAD_MUTEX
-D__const__=__unused__ -D_GNU_SOURCE -DUSE_SYMBOLIZE -DNO_TCMALLOC
-D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS
-DUNIT_TEST -DBVAR_NOT_LINK_DEFAULT_VARIABLES -D__STRICT_ANSI__")
+set(CMAKE_CXX_FLAGS "${CMAKE_CPP_FLAGS} -g -O2 -pipe -Wall -W -fPIC
-fstrict-aliasing -Wno-invalid-offsetof -Wno-unused-parameter
-fno-omit-frame-pointer -fno-access-control")
use_cxx11()
Review Comment:
The PR title/description says the fix is to protect <any> via
sstream_workaround.h, but the actual change removes -Dprivate=public/-include
and switches tests to -fno-access-control instead. Please update the PR
title/description (and/or issue explanation) to match the implemented approach
so reviewers/users understand the behavioral change in test build flags.
##########
test/CMakeLists.txt:
##########
@@ -53,8 +53,8 @@ else()
endif()
set(CMAKE_CPP_FLAGS "${CMAKE_CPP_FLAGS} ${DEFINE_CLOCK_GETTIME}
-DBRPC_WITH_GLOG=${WITH_GLOG_VAL} -DBRPC_WITH_RDMA=${WITH_RDMA_VAL}")
-set(CMAKE_CPP_FLAGS "${CMAKE_CPP_FLAGS} -DBTHREAD_USE_FAST_PTHREAD_MUTEX
-D__const__=__unused__ -D_GNU_SOURCE -DUSE_SYMBOLIZE -DNO_TCMALLOC
-D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS
-DUNIT_TEST -Dprivate=public -Dprotected=public
-DBVAR_NOT_LINK_DEFAULT_VARIABLES -D__STRICT_ANSI__ -include
${PROJECT_SOURCE_DIR}/test/sstream_workaround.h")
-set(CMAKE_CXX_FLAGS "${CMAKE_CPP_FLAGS} -g -O2 -pipe -Wall -W -fPIC
-fstrict-aliasing -Wno-invalid-offsetof -Wno-unused-parameter
-fno-omit-frame-pointer")
+set(CMAKE_CPP_FLAGS "${CMAKE_CPP_FLAGS} -DBTHREAD_USE_FAST_PTHREAD_MUTEX
-D__const__=__unused__ -D_GNU_SOURCE -DUSE_SYMBOLIZE -DNO_TCMALLOC
-D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS
-DUNIT_TEST -DBVAR_NOT_LINK_DEFAULT_VARIABLES -D__STRICT_ANSI__")
+set(CMAKE_CXX_FLAGS "${CMAKE_CPP_FLAGS} -g -O2 -pipe -Wall -W -fPIC
-fstrict-aliasing -Wno-invalid-offsetof -Wno-unused-parameter
-fno-omit-frame-pointer -fno-access-control")
Review Comment:
Removing the global -include of sstream_workaround.h can make the build
depend on transitive inclusion of <sstream> (some test sources use
std::stringstream without including <sstream>). Consider keeping the forced
include (or updating those sources) to avoid brittle builds.
##########
test/Makefile:
##########
@@ -18,8 +18,8 @@
NEED_GPERFTOOLS=1
NEED_GTEST=1
include ../config.mk
-CPPFLAGS+=-DBTHREAD_USE_FAST_PTHREAD_MUTEX -D_GNU_SOURCE -DUSE_SYMBOLIZE
-DNO_TCMALLOC -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
-D__STDC_CONSTANT_MACROS -DUNIT_TEST -Dprivate=public -Dprotected=public
-DBVAR_NOT_LINK_DEFAULT_VARIABLES --include sstream_workaround.h
-CXXFLAGS+=$(CPPFLAGS) -pipe -Wall -W -fPIC -fstrict-aliasing
-Wno-invalid-offsetof -Wno-unused-parameter -fno-omit-frame-pointer -std=c++0x
+CPPFLAGS+=-DBTHREAD_USE_FAST_PTHREAD_MUTEX -D_GNU_SOURCE -DUSE_SYMBOLIZE
-DNO_TCMALLOC -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
-D__STDC_CONSTANT_MACROS -DUNIT_TEST -DBVAR_NOT_LINK_DEFAULT_VARIABLES
+CXXFLAGS+=$(CPPFLAGS) -pipe -Wall -W -fPIC -fstrict-aliasing
-Wno-invalid-offsetof -Wno-unused-parameter -fno-omit-frame-pointer
-fno-access-control -std=c++0x
Review Comment:
Removing the forced include of sstream_workaround.h means test sources that
use std::stringstream without including <sstream> now rely on transitive
includes (e.g., from gtest). To keep the test build robust, either re-add the
forced include or add explicit <sstream> includes to the affected sources.
##########
test/sstream_workaround.h:
##########
@@ -18,16 +18,12 @@
#ifndef BUTIL_TEST_SSTREAM_WORKAROUND
#define BUTIL_TEST_SSTREAM_WORKAROUND
-// defining private as public makes it fail to compile sstream with gcc5.x
like this:
-// "error: ‘struct std::__cxx11::basic_stringbuf<_CharT, _Traits, _Alloc>::
-// __xfer_bufptrs’ redeclared with different access"
-
-#ifdef private
-# undef private
-# include <sstream>
-# define private public
-#else
-# include <sstream>
-#endif
+// Previously, tests used -Dprivate=public to access private members.
+// This caused redeclaration errors with standard library headers (<sstream>,
+// <any>, etc.) that use private access specifiers internally.
+//
+// Now tests use -fno-access-control (compiler flag) instead, which cleanly
+// disables access control without affecting header parsing. This header is
+// kept for backward compatibility but no longer needs the workaround logic.
Review Comment:
sstream_workaround.h is now empty, but test build configurations may still
force-include it. Including <sstream> here (and <any> when building as C++17+)
avoids relying on transitive includes and matches the PR's stated goal of
shielding <any> in C++17 builds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
