THRIFT-2246 Unset enum value is printed by ToString()

Patch: Jens Geyer

2 files changed, 183 insertions, 118 deletions

diff --git a/compiler/cpp/src/generate/t_csharp_generator.cc b/compiler/cpp/src/generate/t_csharp_generator.cc
index 3f432da9b..be2049625 100644
--- a/compiler/cpp/src/generate/t_csharp_generator.cc
+++ b/compiler/cpp/src/generate/t_csharp_generator.cc
@@ -77,13 +77,13 @@ class t_csharp_generator : public t_oop_generator
       iter = parsed_options.find("serial");
       serialize_ = (iter != parsed_options.end());
       if (serialize_) {
-	wcf_namespace_ = iter->second;  // since there can be only one namespace
+        wcf_namespace_ = iter->second;  // since there can be only one namespace
       }
       
       iter = parsed_options.find("wcf");
       wcf_ = (iter != parsed_options.end());
       if (wcf_) {
-	wcf_namespace_ = iter->second;
+        wcf_namespace_ = iter->second;
       }
 
       out_dir_base_ = "gen-csharp";
@@ -278,7 +278,7 @@ void t_csharp_generator::generate_enum(t_enum* tenum) {
   vector<t_enum_value*> constants = tenum->get_constants();
   vector<t_enum_value*>::iterator c_iter;
   for (c_iter = constants.begin(); c_iter != constants.end(); ++c_iter) {
-	generate_csharp_doc(f_enum, *c_iter);
+        generate_csharp_doc(f_enum, *c_iter);
 
     int value = (*c_iter)->get_value();
     indent(f_enum) << (*c_iter)->get_name() << " = " << value << "," << endl;
@@ -575,7 +575,7 @@ void t_csharp_generator::generate_csharp_struct_definition(ofstream &out, t_stru
       // if it is not required, if it has a default value, we need to generate Isset
       // if we are not nullable, then we generate Isset
       if (!is_required && (!nullable_ || has_default)) {
-	indent(out) << "public bool " << (*m_iter)->get_name() << ";" << endl;
+        indent(out) << "public bool " << (*m_iter)->get_name() << ";" << endl;
       }
     }
 
@@ -800,19 +800,19 @@ void t_csharp_generator::generate_csharp_struct_writer(ofstream& out, t_struct*
       bool is_required = field_is_required((*f_iter));
       bool has_default = field_has_default((*f_iter));
       if (nullable_ && !has_default && !is_required) {
-	indent(out) << "if (" << prop_name((*f_iter)) << " != null) {" << endl;
-	indent_up();
+        indent(out) << "if (" << prop_name((*f_iter)) << " != null) {" << endl;
+        indent_up();
       } else if (!is_required) {
-	bool null_allowed = type_can_be_null((*f_iter)->get_type());
-	if (null_allowed) {
-	  indent(out) <<
-	    "if (" << prop_name((*f_iter)) << " != null && __isset." << (*f_iter)->get_name() << ") {" << endl;
-	  indent_up();
-	} else {
-	  indent(out) <<
-	    "if (__isset." << (*f_iter)->get_name() << ") {" << endl;
-	  indent_up();
-	}
+        bool null_allowed = type_can_be_null((*f_iter)->get_type());
+        if (null_allowed) {
+          indent(out) <<
+            "if (" << prop_name((*f_iter)) << " != null && __isset." << (*f_iter)->get_name() << ") {" << endl;
+          indent_up();
+        } else {
+          indent(out) <<
+            "if (__isset." << (*f_iter)->get_name() << ") {" << endl;
+          indent_up();
+        }
       }
       indent(out) << "field.Name = \"" << (*f_iter)->get_name() << "\";" << endl;
       indent(out) << "field.Type = " << type_to_enum((*f_iter)->get_type()) << ";" << endl;
@@ -823,8 +823,8 @@ void t_csharp_generator::generate_csharp_struct_writer(ofstream& out, t_struct*
 
       indent(out) << "oprot.WriteFieldEnd();" << endl;
       if (!is_required) {
-	indent_down();
-	indent(out) << "}" << endl;
+        indent_down();
+        indent(out) << "}" << endl;
       }
     }
   }
@@ -863,10 +863,9 @@ void t_csharp_generator::generate_csharp_struct_result_writer(ofstream& out, t_s
       }
       
       if (nullable_) {
-	out << "(this." << prop_name((*f_iter)) << " != null) {" << endl;
+        out << "(this." << prop_name((*f_iter)) << " != null) {" << endl;
       } else {
-	out <<
-	  "(this.__isset." << (*f_iter)->get_name() << ") {" << endl;
+        out << "(this.__isset." << (*f_iter)->get_name() << ") {" << endl;
       }
       indent_up();
 
@@ -892,8 +891,8 @@ void t_csharp_generator::generate_csharp_struct_result_writer(ofstream& out, t_s
         "oprot.WriteFieldEnd();" << endl;
 
       if (null_allowed) {
-	indent_down();
-	indent(out) << "}" << endl;
+        indent_down();
+        indent(out) << "}" << endl;
       }
 
       indent_down();
@@ -918,36 +917,67 @@ void t_csharp_generator::generate_csharp_struct_tostring(ofstream& out, t_struct
   indent_up();
 
   indent(out) <<
-    "StringBuilder sb = new StringBuilder(\"" << tstruct->get_name() << "(\");" << endl;
+    "StringBuilder __sb = new StringBuilder(\"" << tstruct->get_name() << "(\");" << endl;
 
   const vector<t_field*>& fields = tstruct->get_members();
   vector<t_field*>::const_iterator f_iter;
 
-  bool first = true;
+  indent(out) << "bool __first = true;" << endl;
+  bool had_required = false;  // set to true after first required field has been processed
 
   for (f_iter = fields.begin(); f_iter != fields.end(); ++f_iter) {
-    if (first) {
-      first = false;
+    bool is_required = field_is_required((*f_iter));
+    bool has_default = field_has_default((*f_iter));
+    if (nullable_ && !has_default && !is_required) {
+      indent(out) << "if (" << prop_name((*f_iter)) << " != null) {" << endl;
+      indent_up();
+    } else if (!is_required) {
+      bool null_allowed = type_can_be_null((*f_iter)->get_type());
+      if (null_allowed) {
+        indent(out) <<
+          "if (" << prop_name((*f_iter)) << " != null && __isset." << (*f_iter)->get_name() << ") {" << endl;
+        indent_up();
+      } else {
+        indent(out) <<
+          "if (__isset." << (*f_iter)->get_name() << ") {" << endl;
+        indent_up();
+      }
+    }
+
+    if( ! had_required)	{
+      indent(out) << "if(!__first) { __sb.Append(\", \"); }" << endl;
+	  if( ! is_required) {
+        indent(out) << "__first = false;" << endl;
+	  }
       indent(out) <<
-        "sb.Append(\"" << prop_name((*f_iter)) << ": \");" << endl;
+        "__sb.Append(\"" << prop_name((*f_iter)) << ": \");" << endl;
     } else {
       indent(out) <<
-        "sb.Append(\"," << prop_name((*f_iter)) << ": \");" << endl;
-    }
+        "__sb.Append(\", " << prop_name((*f_iter)) << ": \");" << endl;
+	}
+        
+          
     t_type* ttype = (*f_iter)->get_type();
     if (ttype->is_xception() || ttype->is_struct()) {
       indent(out) <<
-        "sb.Append(" << prop_name((*f_iter)) << "== null ? \"<null>\" : "<< prop_name((*f_iter))  << ".ToString());" << endl;
+        "__sb.Append(" << prop_name((*f_iter)) << "== null ? \"<null>\" : "<< prop_name((*f_iter))  << ".ToString());" << endl;
     } else {
       indent(out) <<
-        "sb.Append(" << prop_name((*f_iter))  << ");" << endl;
+        "__sb.Append(" << prop_name((*f_iter))  << ");" << endl;
+    }
+        
+    if (!is_required) {
+      indent_down();
+      indent(out) << "}" << endl;
+    } else {
+      had_required = true;  // now __first must be false, so we don't need to check it anymore
     }
   }
 
   indent(out) <<
-    "sb.Append(\")\");" << endl;
+    "__sb.Append(\")\");" << endl;
   indent(out) <<
-    "return sb.ToString();" << endl;
+    "return __sb.ToString();" << endl;
 
   indent_down();
   indent(out) << "}" << endl << endl;
@@ -1122,13 +1152,13 @@ void t_csharp_generator::generate_csharp_struct_hashcode(ofstream& out, t_struct
       out << "(!__isset." << (*f_iter)->get_name() << " ? 0 : ";
     }
     if (ttype->is_container()) {
-    	out << "(TCollections.GetHashCode("
-    		<< prop_name((*f_iter))
-    		<< "))";
+            out << "(TCollections.GetHashCode("
+                    << prop_name((*f_iter))
+                    << "))";
     } else {
-		out << "("
-			<< prop_name((*f_iter))
-			<< ".GetHashCode())";
+                out << "("
+                        << prop_name((*f_iter))
+                        << ".GetHashCode())";
     }
     out << ");" << endl;
   }
@@ -1180,8 +1210,8 @@ void t_csharp_generator::generate_service_interface(t_service* tservice) {
   generate_csharp_doc(f_service_, tservice);
 
   if (wcf_) {
-	  indent(f_service_) <<
-		"[ServiceContract(Namespace=\"" << wcf_namespace_ << "\")]" << endl;
+          indent(f_service_) <<
+                "[ServiceContract(Namespace=\"" << wcf_namespace_ << "\")]" << endl;
   }
   indent(f_service_) <<
     "public interface Iface" << extends_iface << " {" << endl;
@@ -1191,19 +1221,19 @@ void t_csharp_generator::generate_service_interface(t_service* tservice) {
   vector<t_function*>::iterator f_iter;
   for (f_iter = functions.begin(); f_iter != functions.end(); ++f_iter)
   {
-	generate_csharp_doc(f_service_, *f_iter);
-
-	// if we're using WCF, add the corresponding attributes
-	if (wcf_) {
-		indent(f_service_) <<
-			"[OperationContract]" << endl;
-
-		const std::vector<t_field*>& xceptions = (*f_iter)->get_xceptions()->get_members();
-		vector<t_field*>::const_iterator x_iter;
-		for (x_iter = xceptions.begin(); x_iter != xceptions.end(); ++x_iter) {
-		  indent(f_service_) << "[FaultContract(typeof(" + type_name((*x_iter)->get_type(), false, false) + "Fault))]" << endl;
-		}
-	}
+        generate_csharp_doc(f_service_, *f_iter);
+
+        // if we're using WCF, add the corresponding attributes
+        if (wcf_) {
+                indent(f_service_) <<
+                        "[OperationContract]" << endl;
+
+                const std::vector<t_field*>& xceptions = (*f_iter)->get_xceptions()->get_members();
+                vector<t_field*>::const_iterator x_iter;
+                for (x_iter = xceptions.begin(); x_iter != xceptions.end(); ++x_iter) {
+                  indent(f_service_) << "[FaultContract(typeof(" + type_name((*x_iter)->get_type(), false, false) + "Fault))]" << endl;
+                }
+        }
 
     indent(f_service_) <<
       function_signature(*f_iter) << ";" << endl;
@@ -1351,7 +1381,7 @@ void t_csharp_generator::generate_service_client(t_service* tservice) {
     f_service_ << ");" << endl;
     scope_down(f_service_);
     f_service_ << endl;
-	
+        
     // End
     indent(f_service_) <<
       "public " << function_signature_async_end(*f_iter, "End_") << endl;
@@ -1373,7 +1403,7 @@ void t_csharp_generator::generate_service_client(t_service* tservice) {
     bool first;
     if( async_||async_ctp_) {
       indent(f_service_) <<
-	"public async " << function_signature_async(*f_iter, "") << endl;
+        "public async " << function_signature_async(*f_iter, "") << endl;
       scope_up(f_service_);
       
       if (!(*f_iter)->get_returntype()->is_void()) {
@@ -1385,9 +1415,9 @@ void t_csharp_generator::generate_service_client(t_service* tservice) {
         indent(f_service_);
       }
       if (async_) {
-	f_service_ << "await Task.Run(() =>" << endl;
+        f_service_ << "await Task.Run(() =>" << endl;
       } else {
-	f_service_ << "await TaskEx.Run(() =>" << endl;
+        f_service_ << "await TaskEx.Run(() =>" << endl;
       }
       scope_up(f_service_);
       indent(f_service_);
@@ -1397,7 +1427,7 @@ void t_csharp_generator::generate_service_client(t_service* tservice) {
       }
       f_service_ << 
         funname << "(";
-	  first = true;
+          first = true;
       for (fld_iter = fields.begin(); fld_iter != fields.end(); ++fld_iter) {
         if (first) {
           first = false;
@@ -1414,7 +1444,7 @@ void t_csharp_generator::generate_service_client(t_service* tservice) {
         indent(f_service_) << 
           "return retval;"  << endl;
       }
-	  scope_down(f_service_);
+          scope_down(f_service_);
       f_service_ << endl;
     }
     
@@ -1551,24 +1581,24 @@ void t_csharp_generator::generate_service_client(t_service* tservice) {
         indent() << "iprot_.ReadMessageEnd();" << endl;
 
       if (!(*f_iter)->get_returntype()->is_void()) {
-	if (nullable_) {
-	  if (type_can_be_null((*f_iter)->get_returntype())) {
-	    f_service_ <<
-	      indent() << "if (result.Success != null) {" << endl <<
-	      indent() << "  return result.Success;" << endl <<
-	      indent() << "}" << endl;
-	  } else {
-	    f_service_ <<
-	      indent() << "if (result.Success.HasValue) {" << endl <<
-	      indent() << "  return result.Success.Value;" << endl <<
-	      indent() << "}" << endl;
-	  }
-	} else {
-	  f_service_ <<
-	    indent() << "if (result.__isset.success) {" << endl <<
-	    indent() << "  return result.Success;" << endl <<
-	    indent() << "}" << endl;
-	}
+        if (nullable_) {
+          if (type_can_be_null((*f_iter)->get_returntype())) {
+            f_service_ <<
+              indent() << "if (result.Success != null) {" << endl <<
+              indent() << "  return result.Success;" << endl <<
+              indent() << "}" << endl;
+          } else {
+            f_service_ <<
+              indent() << "if (result.Success.HasValue) {" << endl <<
+              indent() << "  return result.Success.Value;" << endl <<
+              indent() << "}" << endl;
+          }
+        } else {
+          f_service_ <<
+            indent() << "if (result.__isset.success) {" << endl <<
+            indent() << "  return result.Success;" << endl <<
+            indent() << "}" << endl;
+        }
       }
 
       t_struct *xs = (*f_iter)->get_xceptions();
@@ -1576,17 +1606,17 @@ void t_csharp_generator::generate_service_client(t_service* tservice) {
       const std::vector<t_field*>& xceptions = xs->get_members();
       vector<t_field*>::const_iterator x_iter;
       for (x_iter = xceptions.begin(); x_iter != xceptions.end(); ++x_iter) {
-	if (nullable_) {
-	  f_service_ <<
-	    indent() << "if (result." << prop_name(*x_iter) << " != null) {" << endl <<
-	    indent() << "  throw result." << prop_name(*x_iter) << ";" << endl <<
-	    indent() << "}" << endl;
-	} else {
-	  f_service_ <<
-	    indent() << "if (result.__isset." << (*x_iter)->get_name() << ") {" << endl <<
-	    indent() << "  throw result." << prop_name(*x_iter) << ";" << endl <<
-	    indent() << "}" << endl;
-	}
+        if (nullable_) {
+          f_service_ <<
+            indent() << "if (result." << prop_name(*x_iter) << " != null) {" << endl <<
+            indent() << "  throw result." << prop_name(*x_iter) << ";" << endl <<
+            indent() << "}" << endl;
+        } else {
+          f_service_ <<
+            indent() << "if (result.__isset." << (*x_iter)->get_name() << ") {" << endl <<
+            indent() << "  throw result." << prop_name(*x_iter) << ";" << endl <<
+            indent() << "}" << endl;
+        }
       }
 
       if ((*f_iter)->get_returntype()->is_void()) {
@@ -2246,34 +2276,34 @@ void t_csharp_generator::generate_csharp_property(ofstream& out, t_field* tfield
                   << " " << prop_name(tfield) << " { get; set; }" << endl;
     } else {
       indent(out) << (isPublic ? "public " : "private ") << type_name(tfield->get_type(), false, false, true)
-		  << " " << prop_name(tfield) << endl;
+                  << " " << prop_name(tfield) << endl;
       scope_up(out);
       indent(out) << "get" << endl;
       scope_up(out);
       bool use_nullable = false;
       if (nullable_) {
-	t_type* ttype = tfield->get_type();
-	while (ttype->is_typedef()) {
-	  ttype = ((t_typedef*)ttype)->get_type();
-	}
-	if (ttype->is_base_type()) {
-	  use_nullable = ((t_base_type*)ttype)->get_base() != t_base_type::TYPE_STRING;
-	}
+        t_type* ttype = tfield->get_type();
+        while (ttype->is_typedef()) {
+          ttype = ((t_typedef*)ttype)->get_type();
+        }
+        if (ttype->is_base_type()) {
+          use_nullable = ((t_base_type*)ttype)->get_base() != t_base_type::TYPE_STRING;
+        }
       }
       indent(out) << "return " << fieldPrefix + tfield->get_name() << ";" << endl;
       scope_down(out);
       indent(out) << "set" << endl;
       scope_up(out);
       if (use_nullable) {
-	if (generateIsset) {
-	  indent(out) << "__isset." << tfield->get_name() << " = value.HasValue;" << endl;
-	}
-	indent(out) << "if (value.HasValue) this." << fieldPrefix + tfield->get_name() << " = value.Value;" << endl;
+        if (generateIsset) {
+          indent(out) << "__isset." << tfield->get_name() << " = value.HasValue;" << endl;
+        }
+        indent(out) << "if (value.HasValue) this." << fieldPrefix + tfield->get_name() << " = value.Value;" << endl;
       } else {
-	if (generateIsset) {
-	  indent(out) << "__isset." << tfield->get_name() << " = true;" << endl;
-	}
-	indent(out) << "this." << fieldPrefix + tfield->get_name() << " = value;" << endl;
+        if (generateIsset) {
+          indent(out) << "__isset." << tfield->get_name() << " = true;" << endl;
+        }
+        indent(out) << "this." << fieldPrefix + tfield->get_name() << " = value;" << endl;
       }
       scope_down(out);
       scope_down(out);
@@ -2525,23 +2555,23 @@ void t_csharp_generator::generate_csharp_doc(ofstream &out, t_doc* tdoc) {
 
 void t_csharp_generator::generate_csharp_doc(ofstream &out, t_function* tfunction) {
   if (tfunction->has_doc()) {
-	stringstream ps;
+        stringstream ps;
     const vector<t_field*>& fields = tfunction->get_arglist()->get_members();
     vector<t_field*>::const_iterator p_iter;
     for (p_iter = fields.begin(); p_iter != fields.end(); ++p_iter) {
       t_field* p = *p_iter;
       ps << "\n<param name=\"" << p->get_name() << "\">";
       if (p->has_doc()) {
-		std::string str = p->get_doc();
-		str.erase(std::remove(str.begin(), str.end(), '\n'), str.end()); // remove the newlines that appear from the parser
-		ps << str;
+                std::string str = p->get_doc();
+                str.erase(std::remove(str.begin(), str.end(), '\n'), str.end()); // remove the newlines that appear from the parser
+                ps << str;
       }
-	  ps << "</param>";
+          ps << "</param>";
     }
     generate_docstring_comment(out,
                                "",
                                "/// ",
-							   "<summary>\n" + tfunction->get_doc() + "</summary>" + ps.str(),
+                                                           "<summary>\n" + tfunction->get_doc() + "</summary>" + ps.str(),
                                "");
   }
 }
diff --git a/compiler/cpp/src/generate/t_delphi_generator.cc b/compiler/cpp/src/generate/t_delphi_generator.cc
index 2ad9c70a2..9a865667d 100644
--- a/compiler/cpp/src/generate/t_delphi_generator.cc
+++ b/compiler/cpp/src/generate/t_delphi_generator.cc
@@ -3165,12 +3165,14 @@ void t_delphi_generator::generate_delphi_struct_tostring_impl(ostream& out, stri
     cls_nm = type_name(tstruct,true,false);
   }
 
-  string tmp_sb = "sb";
+  string tmp_sb = "__sb";
+  string tmp_first = "__first";
 
   indent_impl(out) << "function " << cls_prefix << cls_nm << ".ToString: string;" << endl;
   indent_impl(out) << "var" << endl;
   indent_up_impl();
   indent_impl(out) << tmp_sb << " : TThriftStringBuilder;" << endl;
+  indent_impl(out) << tmp_first << " : Boolean;" << endl;
   indent_down_impl();
   indent_impl(out) << "begin" << endl;
   indent_up_impl();
@@ -3179,17 +3181,39 @@ void t_delphi_generator::generate_delphi_struct_tostring_impl(ostream& out, stri
   indent_impl(out) << "try" << endl;
   indent_up_impl();
 
-  bool first = true;
-
+  indent_impl(out) << tmp_first << " := TRUE;" << endl;
+  bool had_required = false;  // set to true after first required field has been processed
+  
   for (f_iter = fields.begin(); f_iter != fields.end(); ++f_iter) {
-    if (first) {
-      first = false;
+    bool null_allowed = type_can_be_null((*f_iter)->get_type());
+    bool is_optional  = ((*f_iter)->get_req() != t_field::T_REQUIRED);
+    if (null_allowed) {
+      indent_impl(out) << "if (" << prop_name((*f_iter), is_exception) << " <> nil)";
+      if (is_optional) {
+        out << " and __isset_" << prop_name(*f_iter,is_exception);
+      }
+      out << " then begin" << endl;
+      indent_up_impl();
+    } else {
+      if (is_optional) {
+        indent_impl(out) << "if (__isset_" << prop_name(*f_iter,is_exception) << ") then begin" << endl;
+        indent_up_impl();
+      }
+    }
+
+    if( ! had_required)	{
+	  indent_impl(out) << "if not " << tmp_first << " then " << tmp_sb << ".Append(',');" << endl;
+	  if (is_optional) {
+        indent_impl(out) << tmp_first << " := FALSE;" << endl;
+      }
       indent_impl(out) <<
         tmp_sb << ".Append('" << prop_name((*f_iter), is_exception) << ": ');" << endl;
     } else {
       indent_impl(out) <<
-        tmp_sb << ".Append('," << prop_name((*f_iter), is_exception) << ": ');" << endl;
+        tmp_sb << ".Append(', " << prop_name((*f_iter), is_exception) << ": ');" << endl;
     }
+	
+
     t_type* ttype = (*f_iter)->get_type();
     if (ttype->is_xception() || ttype->is_struct()) {
       indent_impl(out) <<
@@ -3201,12 +3225,23 @@ void t_delphi_generator::generate_delphi_struct_tostring_impl(ostream& out, stri
       indent_impl(out) <<
         tmp_sb << ".Append(" << prop_name((*f_iter), is_exception)  << ");" << endl;
     }
+	
+    if (null_allowed || is_optional) {
+      indent_down_impl();
+      indent_impl(out) << "end;" << endl;
+    } 
+	
+    if (!is_optional) {
+      had_required = true;  // now __first must be false, so we don't need to check it anymore
+    }
   }
 
   indent_impl(out) <<
     tmp_sb << ".Append(')');" << endl;
   indent_impl(out) <<
     "Result := " << tmp_sb <<  ".ToString;" << endl;
+  indent_impl(out) <<
+    "if " << tmp_first <<  " then {prevent warning};" << endl;
 
   indent_down_impl();
   indent_impl(out) << "finally" << endl;