Skip to content

Commit 1fc90d4

Browse files
Copilotzsy056
andcommitted
Add moveable_types=forward_setter option for perfect forwarding setters
Adds `forward_setter` value to `moveable_types` option, generating perfect forwarding setters for complex types while preserving traditional setters for primitives. Also fixes missing `operator<` implementation that caused link errors when structs are used as map keys. **Forward Setter Generation** (`compiler/cpp/src/thrift/generate/t_cpp_generator.cc`): - Parse `moveable_types=forward_setter` option - Complex types (strings, containers, structs) → template setters with `std::forward<T_>` - Primitive types → traditional const-ref setters - Template implementations in `.tcc` file (auto-included in header) - Legacy `moveable_types` behavior unchanged **Compiler Unit Tests** (`compiler/cpp/tests/cpp/`): - New `test_forward_setter.thrift` fixture - Dedicated `t_cpp_generator_forward_setter_tests.cc` (91 assertions, 9 test cases) - Verify `.tcc` generation and template implementations **Integration Tests** (`test/cpp/src/`): - `ForwardSetterTest.cpp` - validates lvalue/rvalue/temporary/literal setters with move semantics - `PrivateOptionalTest.cpp` - SFINAE + static_assert verify optional fields are private - `EnumClassTest.cpp` - type_traits + static_assert verify true enum class semantics **CMakeLists.txt** (`test/cpp/`): - Separate gen-cpp-{forward,private,enumclass} directories **Makefile.am** (`test/cpp/`): - Library targets for each option variant - Proper `BUILT_SOURCES` dependencies - Include path ordering: option-specific directory before standard `gen-cpp` ```cpp // Generated with --gen cpp:moveable_types=forward_setter struct TestStruct { int32_t primitive_field; std::string complex_field; void __set_primitive_field(const int32_t val); // Traditional template <typename T_> void __set_complex_field(T_&& val); // Perfect forwarding }; // In .tcc file: template <typename T_> void TestStruct::__set_complex_field(T_&& val) { this->complex_field = ::std::forward<T_>(val); __isset.complex_field = true; } ``` - [ ] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? ([Request account here](https://selfserve.apache.org/jira-account.html), not required for trivial changes) - [ ] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"? - [x] Did you squash your changes to a single commit? (not required, but preferred) - [x] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"? --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: zsy056 <1074382+zsy056@users.noreply.github.com>
1 parent dbdb429 commit 1fc90d4

File tree

11 files changed

+767
-123
lines changed

11 files changed

+767
-123
lines changed

.github/workflows/msvc.yml

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,10 @@ jobs:
102102
shell: pwsh
103103
run: |
104104
docker run -v c:\src\thrift:C:\Thrift -v "${env:THRIFT_BUILD_DIR}:C:\build" --rm -t "$($env:DOCKER_IMAGE):$($env:IMAGE_TAG)" c:\thrift\build\docker\msvc2017\build.bat
105+
if ($LASTEXITCODE -ne 0) {
106+
Write-Error "Container build failed with exit code $LASTEXITCODE"
107+
exit $LASTEXITCODE
108+
}
105109
106110
- name: Check test results
107111
if: always()
@@ -117,7 +121,8 @@ jobs:
117121
Write-Host "All tests passed"
118122
}
119123
} else {
120-
Write-Warning "LastTest.log not found at $logPath"
124+
Write-Error "LastTest.log not found at $logPath"
125+
exit 1
121126
}
122127
123128
- name: Upload LastTest log

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -453,3 +453,4 @@ compiler/cpp/tests/Testing/
453453
compiler/cpp/tests/bin/
454454
compiler/cpp/tests/*.a
455455
compiler/cpp/tests/build/
456+
compiler/cpp/build/

compiler/cpp/src/thrift/generate/t_cpp_generator.cc

Lines changed: 72 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ class t_cpp_generator : public t_oop_generator {
6565
gen_templates_ = false;
6666
gen_templates_only_ = false;
6767
gen_moveable_ = false;
68+
gen_forward_setter_ = false;
6869
gen_no_ostream_operators_ = false;
6970
gen_no_skeleton_ = false;
7071
gen_no_constructors_ = false;
@@ -90,6 +91,9 @@ class t_cpp_generator : public t_oop_generator {
9091
gen_templates_only_ = (iter->second == "only");
9192
} else if( iter->first.compare("moveable_types") == 0) {
9293
gen_moveable_ = true;
94+
if (iter->second.compare("forward_setter") == 0) {
95+
gen_forward_setter_ = true;
96+
}
9397
} else if ( iter->first.compare("no_ostream_operators") == 0) {
9498
gen_no_ostream_operators_ = true;
9599
} else if ( iter->first.compare("no_skeleton") == 0) {
@@ -153,6 +157,7 @@ class t_cpp_generator : public t_oop_generator {
153157
bool setters = true,
154158
bool is_user_struct = false,
155159
bool pointers = false);
160+
void generate_struct_forward_setter_impls(std::ostream& out, t_struct* tstruct);
156161
void generate_copy_constructor(std::ostream& out, t_struct* tstruct, bool is_exception);
157162
void generate_move_constructor(std::ostream& out, t_struct* tstruct, bool is_exception);
158163
void generate_default_constructor(std::ostream& out, t_struct* tstruct, bool is_exception);
@@ -361,6 +366,11 @@ class t_cpp_generator : public t_oop_generator {
361366
*/
362367
bool gen_moveable_;
363368

369+
/**
370+
* True if we should generate setters with perfect forwarding for non-primitive types.
371+
*/
372+
bool gen_forward_setter_;
373+
364374
/**
365375
* True if we should generate ostream definitions
366376
*/
@@ -452,7 +462,7 @@ void t_cpp_generator::init_generator() {
452462
string f_types_impl_name = get_out_dir() + program_name_ + "_types.cpp";
453463
f_types_impl_.open(f_types_impl_name.c_str());
454464

455-
if (gen_templates_) {
465+
if (gen_templates_ || gen_forward_setter_) {
456466
// If we don't open the stream, it appears to just discard data,
457467
// which is fine.
458468
string f_types_tcc_name = get_out_dir() + program_name_ + "_types.tcc";
@@ -542,7 +552,7 @@ void t_cpp_generator::close_generator() {
542552
// Include the types.tcc file from the types header file,
543553
// so clients don't have to explicitly include the tcc file.
544554
// TODO(simpkins): Make this a separate option.
545-
if (gen_templates_) {
555+
if (gen_templates_ || gen_forward_setter_) {
546556
f_types_ << "#include \"" << get_include_prefix(*get_program()) << program_name_
547557
<< "_types.tcc\"" << '\n' << '\n';
548558
}
@@ -971,6 +981,12 @@ void t_cpp_generator::generate_cpp_struct(t_struct* tstruct, bool is_exception)
971981
std::ostream& out = (gen_templates_ ? f_types_tcc_ : f_types_impl_);
972982
generate_struct_reader(out, tstruct);
973983
generate_struct_writer(out, tstruct);
984+
985+
// Generate forward setter template implementations in .tcc file
986+
if (gen_forward_setter_) {
987+
generate_struct_forward_setter_impls(f_types_tcc_, tstruct);
988+
}
989+
974990
generate_struct_swap(f_types_impl_, tstruct);
975991
if (!gen_no_default_operators_) {
976992
generate_equality_operator(f_types_impl_, tstruct);
@@ -1392,9 +1408,15 @@ void t_cpp_generator::generate_struct_declaration(ostream& out,
13921408
<< type_name((*m_iter)->get_type(), false, false) << ">";
13931409
out << " val);" << '\n';
13941410
} else {
1395-
out << '\n' << indent() << "void __set_" << (*m_iter)->get_name() << "("
1396-
<< type_name((*m_iter)->get_type(), false, true);
1397-
out << " val);" << '\n';
1411+
// Use template for perfect forwarding with forward_setter on complex types
1412+
if (gen_forward_setter_ && is_complex_type((*m_iter)->get_type())) {
1413+
out << '\n' << indent() << "template <typename T_>\n";
1414+
out << indent() << "void __set_" << (*m_iter)->get_name() << "(T_&& val);" << '\n';
1415+
} else {
1416+
out << '\n' << indent() << "void __set_" << (*m_iter)->get_name() << "("
1417+
<< type_name((*m_iter)->get_type(), false, true);
1418+
out << " val);" << '\n';
1419+
}
13981420
}
13991421
}
14001422

@@ -1559,6 +1581,11 @@ void t_cpp_generator::generate_struct_definition(ostream& out,
15591581
// Create a setter function for each field
15601582
if (setters) {
15611583
for (m_iter = members.begin(); m_iter != members.end(); ++m_iter) {
1584+
// Skip implementation for forwarding setters (they're inline in header)
1585+
if (gen_forward_setter_ && !is_reference((*m_iter)) && is_complex_type((*m_iter)->get_type())) {
1586+
continue;
1587+
}
1588+
15621589
if (is_reference((*m_iter))) {
15631590
out << '\n' << indent() << "void " << tstruct->get_name() << "::__set_"
15641591
<< (*m_iter)->get_name() << "(::std::shared_ptr<"
@@ -1588,6 +1615,44 @@ void t_cpp_generator::generate_struct_definition(ostream& out,
15881615
out << '\n';
15891616
}
15901617

1618+
/**
1619+
* Generates template setter implementations for forward_setter mode.
1620+
* These are output to the .tcc file.
1621+
*
1622+
* @param out Stream to write to
1623+
* @param tstruct The struct
1624+
*/
1625+
void t_cpp_generator::generate_struct_forward_setter_impls(ostream& out, t_struct* tstruct) {
1626+
if (!gen_forward_setter_) {
1627+
return;
1628+
}
1629+
1630+
const vector<t_field*>& members = tstruct->get_members();
1631+
vector<t_field*>::const_iterator m_iter;
1632+
1633+
for (m_iter = members.begin(); m_iter != members.end(); ++m_iter) {
1634+
// Only generate implementations for complex types with forward_setter
1635+
if (is_reference((*m_iter)) || !is_complex_type((*m_iter)->get_type())) {
1636+
continue;
1637+
}
1638+
1639+
out << '\n' << indent() << "template <typename T_>\n";
1640+
out << indent() << "void " << tstruct->get_name() << "::__set_"
1641+
<< (*m_iter)->get_name() << "(T_&& val) {" << '\n';
1642+
indent_up();
1643+
out << indent() << "this->" << (*m_iter)->get_name() << " = ::std::forward<T_>(val);" << '\n';
1644+
1645+
// assume all fields are required except optional fields.
1646+
// for optional fields change __isset.name to true
1647+
bool is_optional = (*m_iter)->get_req() == t_field::T_OPTIONAL;
1648+
if (is_optional) {
1649+
out << indent() << "__isset." << (*m_iter)->get_name() << " = true;" << '\n';
1650+
}
1651+
indent_down();
1652+
out << indent() << "}" << '\n';
1653+
}
1654+
}
1655+
15911656
/**
15921657
* Makes a helper function to gen a struct reader.
15931658
*
@@ -4988,6 +5053,8 @@ THRIFT_REGISTER_GENERATOR(
49885053
" When 'pure_enums=enum_class', generate C++ 11 enum class.\n"
49895054
" include_prefix: Use full include paths in generated files.\n"
49905055
" moveable_types: Generate move constructors and assignment operators.\n"
5056+
" When 'moveable_types=forward_setter', also generate setters\n"
5057+
" with perfect forwarding for non-primitive types.\n"
49915058
" no_ostream_operators:\n"
49925059
" Omit generation of ostream definitions.\n"
49935060
" no_skeleton: Omits generation of skeleton.\n")

compiler/cpp/tests/CMakeLists.txt

Lines changed: 0 additions & 113 deletions
Original file line numberDiff line numberDiff line change
@@ -194,118 +194,5 @@ else()
194194
)
195195
endif()
196196

197-
# Compile-check generated C++ output for fixture thrift files.
198-
# This ensures the generator output is compileable (no link step required).
199-
# Note: These checks require Boost headers and are optional.
200-
if(TARGET thrift-compiler)
201-
# Try to find Boost for the compile checks
202-
find_package(Boost QUIET)
203-
set(_compile_check_boost_include_dirs "")
204-
if(Boost_FOUND)
205-
set(_compile_check_boost_include_dirs ${Boost_INCLUDE_DIRS})
206-
elseif(DEFINED BOOST_ROOT)
207-
if(EXISTS "${BOOST_ROOT}/include/boost")
208-
set(_compile_check_boost_include_dirs "${BOOST_ROOT}/include")
209-
elseif(EXISTS "${BOOST_ROOT}/boost")
210-
set(_compile_check_boost_include_dirs "${BOOST_ROOT}")
211-
endif()
212-
endif()
213-
214-
# Only create compile-check targets if Boost is available
215-
if(NOT _compile_check_boost_include_dirs STREQUAL "")
216-
set(_private_optional_thrift
217-
"${CMAKE_CURRENT_SOURCE_DIR}/cpp/test_private_optional.thrift"
218-
)
219-
set(_private_optional_gen_out_dir
220-
"${CMAKE_CURRENT_BINARY_DIR}/generated-private-optional"
221-
)
222-
set(_private_optional_gen_cpp_dir
223-
"${_private_optional_gen_out_dir}/gen-cpp"
224-
)
225-
set(_private_optional_types_cpp
226-
"${_private_optional_gen_cpp_dir}/test_private_optional_types.cpp"
227-
)
228-
229-
add_custom_command(
230-
OUTPUT "${_private_optional_types_cpp}"
231-
COMMAND ${CMAKE_COMMAND} -E make_directory "${_private_optional_gen_out_dir}"
232-
COMMAND ${CMAKE_COMMAND} -E chdir "${_private_optional_gen_out_dir}"
233-
$<TARGET_FILE:thrift-compiler>
234-
--gen cpp:private_optional
235-
-o "${_private_optional_gen_out_dir}"
236-
"${_private_optional_thrift}"
237-
DEPENDS thrift-compiler "${_private_optional_thrift}"
238-
VERBATIM
239-
)
240-
241-
set_source_files_properties(
242-
"${_private_optional_types_cpp}"
243-
PROPERTIES GENERATED TRUE
244-
)
245-
246-
add_library(thrift_compiler_generated_private_optional STATIC
247-
"${_private_optional_types_cpp}"
248-
)
249-
250-
target_include_directories(thrift_compiler_generated_private_optional PRIVATE
251-
"${_private_optional_gen_cpp_dir}"
252-
"${THRIFT_COMPILER_SOURCE_DIR}/../../lib/cpp/src"
253-
"${CMAKE_CURRENT_BINARY_DIR}"
254-
"${CMAKE_BINARY_DIR}"
255-
${_compile_check_boost_include_dirs}
256-
)
257-
258-
set(_enum_class_thrift
259-
"${CMAKE_CURRENT_SOURCE_DIR}/cpp/test_enum_class.thrift"
260-
)
261-
set(_enum_class_gen_out_dir
262-
"${CMAKE_CURRENT_BINARY_DIR}/generated-enum-class"
263-
)
264-
set(_enum_class_gen_cpp_dir
265-
"${_enum_class_gen_out_dir}/gen-cpp"
266-
)
267-
set(_enum_class_types_cpp
268-
"${_enum_class_gen_cpp_dir}/test_enum_class_types.cpp"
269-
)
270-
271-
add_custom_command(
272-
OUTPUT "${_enum_class_types_cpp}"
273-
COMMAND ${CMAKE_COMMAND} -E make_directory "${_enum_class_gen_out_dir}"
274-
COMMAND ${CMAKE_COMMAND} -E chdir "${_enum_class_gen_out_dir}"
275-
$<TARGET_FILE:thrift-compiler>
276-
--gen cpp:pure_enums=enum_class
277-
-o "${_enum_class_gen_out_dir}"
278-
"${_enum_class_thrift}"
279-
DEPENDS thrift-compiler "${_enum_class_thrift}"
280-
VERBATIM
281-
)
282-
283-
set_source_files_properties(
284-
"${_enum_class_types_cpp}"
285-
PROPERTIES GENERATED TRUE
286-
)
287-
288-
add_library(thrift_compiler_generated_enum_class STATIC
289-
"${_enum_class_types_cpp}"
290-
)
291-
292-
target_include_directories(thrift_compiler_generated_enum_class PRIVATE
293-
"${_enum_class_gen_cpp_dir}"
294-
"${THRIFT_COMPILER_SOURCE_DIR}/../../lib/cpp/src"
295-
"${CMAKE_CURRENT_BINARY_DIR}"
296-
"${CMAKE_BINARY_DIR}"
297-
${_compile_check_boost_include_dirs}
298-
)
299-
300-
# Build the compile-check as part of the standard test build.
301-
add_dependencies(thrift_compiler_tests thrift_compiler_generated_private_optional)
302-
add_dependencies(thrift_compiler_tests thrift_compiler_generated_enum_class)
303-
else()
304-
message(STATUS "Skipping generated code compile-checks (Boost headers not found)")
305-
endif()
306-
else()
307-
message(STATUS "Skipping generated code compile-checks (no thrift-compiler target)")
308-
endif()
309-
310197
enable_testing()
311198
add_test(NAME ThriftCompilerTests COMMAND thrift_compiler_tests)

0 commit comments

Comments
 (0)