From: Jens Geyer Date: Fri, 2 May 2014 19:31:55 +0000 (+0200) Subject: THRIFT-2471 Make cpp.ref annotation language agnostic X-Git-Url: https://source.supwisdom.com/gerrit/gitweb?a=commitdiff_plain;h=885c6791235e011794a0b65c00f1d9fdf3d233e3;p=common%2Fthrift.git THRIFT-2471 Make cpp.ref annotation language agnostic Client: compiler general Patch: Dave Watson This closes #113 commit 52b99af4ee1574253dcb77933d76a7ebb2d830df Author: Dave Watson Date: 2014-04-23T20:05:56Z change cpp.ref to & commit 3f9d31cc6140367529fd8f7b1b67056ec321786f Author: Dave Watson Date: 2014-04-23T21:50:29Z Recursion depth limit commit 61468e4534ce9e6a4f4f643bfd00542d13600d83 Author: Dave Watson Date: 2014-04-25T19:59:18Z shared_ptr for reference type --- diff --git a/compiler/cpp/src/generate/t_cpp_generator.cc b/compiler/cpp/src/generate/t_cpp_generator.cc index 92eab2f0..f985492c 100755 --- a/compiler/cpp/src/generate/t_cpp_generator.cc +++ b/compiler/cpp/src/generate/t_cpp_generator.cc @@ -235,7 +235,7 @@ class t_cpp_generator : public t_oop_generator { void generate_local_reflection_pointer(std::ofstream& out, t_type* ttype); bool is_reference(t_field* tfield) { - return tfield->annotations_.count("cpp.ref") != 0; + return tfield->get_reference(); } bool is_complex_type(t_type* ttype) { @@ -832,14 +832,8 @@ void t_cpp_generator::generate_copy_constructor( const vector& members = tstruct->get_members(); vector::const_iterator f_iter; for (f_iter = members.begin(); f_iter != members.end(); ++f_iter) { - if (is_reference(*f_iter)) { - std::string type = type_name((*f_iter)->get_type()); - indent(out) << (*f_iter)->get_name() << " = new " << type << "(*" << tmp_name << "." << - (*f_iter)->get_name() << ");" << endl; - } else { - indent(out) << (*f_iter)->get_name() << " = " << tmp_name << "." << - (*f_iter)->get_name() << ";" << endl; - } + indent(out) << (*f_iter)->get_name() << " = " << tmp_name << "." << + (*f_iter)->get_name() << ";" << endl; } indent_down(); @@ -859,20 +853,8 @@ void t_cpp_generator::generate_assignment_operator( const vector& members = tstruct->get_members(); vector::const_iterator f_iter; for (f_iter = members.begin(); f_iter != members.end(); ++f_iter) { - if (is_reference(*f_iter)) { - std::string type = type_name((*f_iter)->get_type()); - indent(out) << "if (this == &" << tmp_name << ") return *this;" << endl; - indent(out) << "if (" << (*f_iter)->get_name() << ") {" << endl; - indent(out) << " *" << (*f_iter)->get_name() << " = *" << tmp_name << "." << - (*f_iter)->get_name() << ";" << endl; - indent(out) << "} else {" << endl; - indent(out) << " " << (*f_iter)->get_name() << " = new " << type << "(*" << tmp_name << "." << - (*f_iter)->get_name() << ");" << endl; - indent(out) << "}" << endl; - } else { - indent(out) << (*f_iter)->get_name() << " = " << tmp_name << "." << - (*f_iter)->get_name() << ";" << endl; - } + indent(out) << (*f_iter)->get_name() << " = " << tmp_name << "." << + (*f_iter)->get_name() << ";" << endl; } indent(out) << "return *this;" << endl; @@ -1031,7 +1013,7 @@ void t_cpp_generator::generate_struct_declaration(ofstream& out, // Declare all fields for (m_iter = members.begin(); m_iter != members.end(); ++m_iter) { indent(out) << - declare_field(*m_iter, false, (pointers && !(*m_iter)->get_type()->is_xception()) || is_reference(*m_iter), !read) << endl; + declare_field(*m_iter, false, (pointers && !(*m_iter)->get_type()->is_xception()), !read) << endl; } // Add the __isset data member if we need it, using the definition from above @@ -1046,11 +1028,19 @@ void t_cpp_generator::generate_struct_declaration(ofstream& out, if (pointers) { continue; } - out << - endl << - indent() << "void __set_" << (*m_iter)->get_name() << + if (is_reference((*m_iter))) { + out << + endl << + indent() << "void __set_" << (*m_iter)->get_name() << + "(boost::shared_ptr<" << type_name((*m_iter)->get_type(), false, false) << ">"; + out << " val);" << endl; + } else { + out << + endl << + indent() << "void __set_" << (*m_iter)->get_name() << "(" << type_name((*m_iter)->get_type(), false, true); - out << " val);" << endl; + out << " val);" << endl; + } } out << endl; @@ -1151,13 +1141,6 @@ void t_cpp_generator::generate_struct_definition(ofstream& out, indent() << tstruct->get_name() << "::~" << tstruct->get_name() << "() throw() {" << endl; indent_up(); - for (m_iter = members.begin(); m_iter != members.end(); ++m_iter) { - if (is_reference(*m_iter)) { - out << indent() << - "delete " << (*m_iter)->get_name() << ";" << endl; - } - } - indent_down(); out << indent() << "}" << endl << endl; } @@ -1165,22 +1148,22 @@ void t_cpp_generator::generate_struct_definition(ofstream& out, // Create a setter function for each field if (setters) { for (m_iter = members.begin(); m_iter != members.end(); ++m_iter) { - out << - endl << - indent() << "void " << tstruct->get_name() << "::__set_" << (*m_iter)->get_name() << - "(" << type_name((*m_iter)->get_type(), false, true); - out << " val) {" << endl; - indent_up(); if (is_reference((*m_iter))) { std::string type = type_name((*m_iter)->get_type()); - indent(out) << "if (" << (*m_iter)->get_name() << ") {" << endl; - indent(out) << " *" << (*m_iter)->get_name() << " = val;" << endl; - indent(out) << "} else {" << endl; - indent(out) << " " << (*m_iter)->get_name() << " = new " << type << "(val);" << endl; - indent(out) << "}" << endl; + out << + endl << + indent() << "void " << tstruct->get_name() << "::__set_" << (*m_iter)->get_name() << + "(boost::shared_ptr<" << type_name((*m_iter)->get_type(), false, false) << ">"; + out << " val) {" << endl; } else { - out << indent() << (*m_iter)->get_name() << " = val;" << endl; + out << + endl << + indent() << "void " << tstruct->get_name() << "::__set_" << (*m_iter)->get_name() << + "(" << type_name((*m_iter)->get_type(), false, true); + out << " val) {" << endl; } + indent_up(); + out << indent() << (*m_iter)->get_name() << " = val;" << endl; indent_down(); // assume all fields are required except optional fields. @@ -1545,6 +1528,7 @@ void t_cpp_generator::generate_struct_writer(ofstream& out, out << indent() << "uint32_t xfer = 0;" << endl; + indent(out) << "oprot->incrementRecursionDepth();" << endl; indent(out) << "xfer += oprot->writeStructBegin(\"" << name << "\");" << endl; @@ -1585,6 +1569,7 @@ void t_cpp_generator::generate_struct_writer(ofstream& out, out << indent() << "xfer += oprot->writeFieldStop();" << endl << indent() << "xfer += oprot->writeStructEnd();" << endl << + indent() << "oprot->decrementRecursionDepth();" << endl << indent() << "return xfer;" << endl; indent_down(); @@ -4078,7 +4063,7 @@ void t_cpp_generator::generate_deserialize_struct(ofstream& out, bool pointer) { if (pointer) { indent(out) << "if (!" << prefix << ") { " << endl; - indent(out) << " " << prefix << " = new " << type_name(tstruct) << ";" << endl; + indent(out) << " " << prefix << " = boost::shared_ptr<" << type_name(tstruct) << ">(new " << type_name(tstruct) << ");" << endl; indent(out) << "}" << endl; indent(out) << "xfer += " << prefix << "->read(iprot);" << endl; @@ -4614,6 +4599,9 @@ string t_cpp_generator::declare_field(t_field* tfield, bool init, bool pointer, result += "const "; } result += type_name(tfield->get_type()); + if (is_reference(tfield)) { + result = "boost::shared_ptr<" + result + ">"; + } if (pointer) { result += "*"; } diff --git a/compiler/cpp/src/parse/t_field.h b/compiler/cpp/src/parse/t_field.h index 7bbcc0f3..c05fdf37 100644 --- a/compiler/cpp/src/parse/t_field.h +++ b/compiler/cpp/src/parse/t_field.h @@ -42,7 +42,8 @@ class t_field : public t_doc { value_(NULL), xsd_optional_(false), xsd_nillable_(false), - xsd_attrs_(NULL) {} + xsd_attrs_(NULL), + reference_(false) {} t_field(t_type* type, std::string name, int32_t key) : type_(type), @@ -52,7 +53,8 @@ class t_field : public t_doc { value_(NULL), xsd_optional_(false), xsd_nillable_(false), - xsd_attrs_(NULL) {} + xsd_attrs_(NULL), + reference_(false) {} ~t_field() {} @@ -137,6 +139,14 @@ class t_field : public t_doc { std::map annotations_; + bool get_reference() { + return reference_; + } + + void set_reference(bool reference) { + reference_ = reference; + } + private: t_type* type_; std::string name_; @@ -147,7 +157,7 @@ class t_field : public t_doc { bool xsd_optional_; bool xsd_nillable_; t_struct* xsd_attrs_; - + bool reference_; }; /** diff --git a/compiler/cpp/src/thriftl.ll b/compiler/cpp/src/thriftl.ll index 685bb543..aee44060 100644 --- a/compiler/cpp/src/thriftl.ll +++ b/compiler/cpp/src/thriftl.ll @@ -183,6 +183,7 @@ literal_begin (['\"]) pwarning(0, "\"async\" is deprecated. It is called \"oneway\" now.\n"); return tok_oneway; } +"&" { return tok_reference; } "BEGIN" { thrift_reserved_keyword(yytext); } diff --git a/compiler/cpp/src/thrifty.yy b/compiler/cpp/src/thrifty.yy index edb05f90..62e13bad 100644 --- a/compiler/cpp/src/thrifty.yy +++ b/compiler/cpp/src/thrifty.yy @@ -165,6 +165,7 @@ const int struct_is_union = 1; %token tok_required %token tok_optional %token tok_union +%token tok_reference /** * Grammar nodes @@ -193,6 +194,7 @@ const int struct_is_union = 1; %type FieldType %type FieldValue %type FieldList +%type FieldReference %type Enum %type EnumDefList @@ -955,35 +957,36 @@ FieldList: } Field: - CaptureDocText FieldIdentifier FieldRequiredness FieldType tok_identifier FieldValue XsdOptional XsdNillable XsdAttributes TypeAnnotations CommaOrSemicolonOptional + CaptureDocText FieldIdentifier FieldRequiredness FieldType FieldReference tok_identifier FieldValue XsdOptional XsdNillable XsdAttributes TypeAnnotations CommaOrSemicolonOptional { pdebug("tok_int_constant : Field -> FieldType tok_identifier"); if ($2.auto_assigned) { - pwarning(1, "No field key specified for %s, resulting protocol may have conflicts or not be backwards compatible!\n", $5); + pwarning(1, "No field key specified for %s, resulting protocol may have conflicts or not be backwards compatible!\n", $6); if (g_strict >= 192) { yyerror("Implicit field keys are deprecated and not allowed with -strict"); exit(1); } } - validate_simple_identifier($5); - $$ = new t_field($4, $5, $2.value); + validate_simple_identifier($6); + $$ = new t_field($4, $6, $2.value); + $$->set_reference($5); $$->set_req($3); - if ($6 != NULL) { - g_scope->resolve_const_value($6, $4); - validate_field_value($$, $6); - $$->set_value($6); + if ($7 != NULL) { + g_scope->resolve_const_value($7, $4); + validate_field_value($$, $7); + $$->set_value($7); } - $$->set_xsd_optional($7); - $$->set_xsd_nillable($8); + $$->set_xsd_optional($8); + $$->set_xsd_nillable($9); if ($1 != NULL) { $$->set_doc($1); } - if ($9 != NULL) { - $$->set_xsd_attrs($9); - } if ($10 != NULL) { - $$->annotations_ = $10->annotations_; - delete $10; + $$->set_xsd_attrs($10); + } + if ($11 != NULL) { + $$->annotations_ = $11->annotations_; + delete $11; } } @@ -1029,6 +1032,16 @@ FieldIdentifier: $$.auto_assigned = true; } +FieldReference: + tok_reference + { + $$ = true; + } +| + { + $$ = false; + } + FieldRequiredness: tok_required { diff --git a/lib/cpp/src/thrift/protocol/TProtocol.h b/lib/cpp/src/thrift/protocol/TProtocol.h index e72033ac..e8ba429d 100644 --- a/lib/cpp/src/thrift/protocol/TProtocol.h +++ b/lib/cpp/src/thrift/protocol/TProtocol.h @@ -283,6 +283,8 @@ uint32_t skip(Protocol_& prot, TType type) { return 0; } +static const uint32_t DEFAULT_RECURSION_LIMIT = 64; + /** * Abstract class for a thrift protocol driver. These are all the methods that * a protocol must implement. Essentially, there must be some way of reading @@ -660,15 +662,28 @@ class TProtocol { return ptrans_; } - protected: - TProtocol(boost::shared_ptr ptrans): - ptrans_(ptrans) { + void incrementRecursionDepth() { + if (recursion_limit_ < ++recursion_depth_) { + throw TProtocolException(TProtocolException::DEPTH_LIMIT); + } } + void decrementRecursionDepth() { + --recursion_depth_; + } + + protected: + TProtocol(boost::shared_ptr ptrans) + : ptrans_(ptrans) + , recursion_depth_(0) + , recursion_limit_(DEFAULT_RECURSION_LIMIT) {} + boost::shared_ptr ptrans_; private: TProtocol() {} + uint32_t recursion_depth_; + uint32_t recursion_limit_; }; /** diff --git a/lib/cpp/src/thrift/protocol/TProtocolException.h b/lib/cpp/src/thrift/protocol/TProtocolException.h index a03d3c8c..4ddb81e0 100644 --- a/lib/cpp/src/thrift/protocol/TProtocolException.h +++ b/lib/cpp/src/thrift/protocol/TProtocolException.h @@ -45,6 +45,7 @@ class TProtocolException : public apache::thrift::TException { , SIZE_LIMIT = 3 , BAD_VERSION = 4 , NOT_IMPLEMENTED = 5 + , DEPTH_LIMIT = 6 }; TProtocolException() : diff --git a/lib/cpp/test/RecursiveTest.cpp b/lib/cpp/test/RecursiveTest.cpp index 00610c6c..24c0f7cd 100644 --- a/lib/cpp/test/RecursiveTest.cpp +++ b/lib/cpp/test/RecursiveTest.cpp @@ -44,7 +44,7 @@ int main() { assert(tree == result); RecList l; - RecList* l2(new RecList); + boost::shared_ptr l2(new RecList); l.nextitem = l2; l.write(prot.get()); @@ -55,7 +55,7 @@ int main() { assert(resultlist.nextitem->nextitem == NULL); CoRec c; - CoRec2* r(new CoRec2); + boost::shared_ptr r(new CoRec2); c.other = r; c.write(prot.get()); @@ -64,4 +64,12 @@ int main() { assert(c.other != NULL); assert(c.other->other.other == NULL); + boost::shared_ptr depthLimit(new RecList); + depthLimit->nextitem = depthLimit; + try { + depthLimit->write(prot.get()); + assert(false); + } catch (const apache::thrift::protocol::TProtocolException& e) { + } + } diff --git a/test/Recursive.thrift b/test/Recursive.thrift index c55541be..9c29983c 100644 --- a/test/Recursive.thrift +++ b/test/Recursive.thrift @@ -23,12 +23,12 @@ struct RecTree { } struct RecList { - 1: RecList nextitem (cpp.ref = "true") + 1: RecList & nextitem 3: i16 item } struct CoRec { - 1: CoRec2 other (cpp.ref = "true") + 1: CoRec2 & other } struct CoRec2 {