Re: DavOrgan C++ port - please review (sfidl changes)

[Stefan Westerfeld <stefan space twc de>]

   Hi!

Here is a new version that contains the sfidl changes only.

On Wed, Nov 15, 2006 at 02:36:12AM +0100, Tim Janik wrote:
> > Index: sfi/sfidl-parser.cc
> > ===================================================================
> > --- sfi/sfidl-parser.cc	(revision 4015)
> > +++ sfi/sfidl-parser.cc	(working copy)
> > @@ -37,6 +37,14 @@ Sfidl::string_from_int (long long int ll
> >   return str;
> > }
> >
> > +// we don't need that (yet) in the Sfidl namespace
> > +static std::string
> > +string_from_double (double value)
> > +{
> > +  char buffer[G_ASCII_DTOSTR_BUF_SIZE + 1] = "";
> > +  g_ascii_formatd (buffer, G_ASCII_DTOSTR_BUF_SIZE, "%.17e", value);
> > +  return buffer;
> > +}
> 
> there is a Birnet funciton named like this already, re-using that name
> just makes the below code highly confusing (because it's hard to tell
> at which place which funciton gets called).
> either the above fix should go into the birnet variant, or the above
> function should be renamed.

I renamed the function. For those cases where my functions where just
reimplementing birnet variants, I am using those now.

> > namespace {
> > using namespace Sfidl;
> > @@ -997,16 +1005,13 @@ GTokenType Parser::parseStringOrConst (s
> > 	  if (ci->name == s &&
> >               ci->type != Constant::tIdent)     /* identifier constants can't be expanded to strings */
> > 	    {
> > -	      char *x = 0;
> > 	      switch (ci->type)
> > 		{
> > 		  case Constant::tInt:
> > -		    s = x = g_strdup_printf ("%lldLL", ci->i);
> > -		    g_free (x);
> > +		    s = string_from_int (ci->i) + "LL";
> > 		    break;
> > 		  case Constant::tFloat:
> > -		    s = x = g_strdup_printf ("%.17g", ci->f);
> > -		    g_free (x);
> > +		    s = string_from_double (ci->f);
> > 		    break;
> > 		  case Constant::tString:
> > 		    s = ci->str;
> > @@ -1522,13 +1527,13 @@ GTokenType Parser::parseParamHints (Para
> >   while (!g_scanner_eof (scanner) && bracelevel > 0)
> >     {
> >       GTokenType t = scanner_get_next_token_with_colon_identifiers (scanner);
> > -      gchar *token_as_string = 0, *x = 0;
> > +      string token_as_string;
> >       bool current_arg_complete = false;
> > +      char *tmp = 0;
> 
> telling from the use below, declaring tmp without initialization
> could help the compiler catch programming errors.
> 
> also, it should be moved into the innermost scope (that is a general rule
> we apply), i.e. inside the switch {} or case {} braces.

Ok.

> >       if(int(t) > 0 && int(t) <= 255)
> > 	{
> > -	  token_as_string = g_new0 (char, 2); /* FIXME: leak */
> > -	  token_as_string[0] = char(t);
> > +	  token_as_string = char(t);
> 
> we put spaces before parenthesis in function calls.

Ok.

Index: sfi/ChangeLog
===================================================================
--- sfi/ChangeLog	(revision 4138)
+++ sfi/ChangeLog	(working copy)
@@ -1,3 +1,24 @@
+Mon Dec 11 19:19:55 2006  Stefan Westerfeld  <stefan space twc de>
+
+	* sfidl-parser.cc: Don't use g_strdup_printf, but g_ascii_formatd for
+	formatting doubles (locale independancy). Some simplification could be
+	achieved by switching from char* to std::string for some code.
+	Switched double format from "%.17g" to "%.17e". This keeps doubles in
+	param specs like (50.0/100.0) in tact, because the C++ compiler will
+	still recognize that we have doubles here. 
+	The old code would print (50/100), because "%g" omits decimal point
+	and trailing digits for those doubles that can be represented without,
+	so that the result of the division (when evaluated by the C++ compiler)
+	would be 0, instead of the expected 0.5.
+
+	* sfidl-parser.hh:
+	* sfidl-parser.cc:
+	* sfidl-corecxx.cc: Use birnet functions to convert between strings
+	and integers, instead of reimplementing our own version.
+
+	* Makefile.am: Start linking sfidl against birnet, as we use birnet
+	string helper functions now.
+
 Sun Dec 3 13:47:31 2006  Stefan Westerfeld  <stefan space twc de>
 
 	* sfidl-clientcxx.cc:
Index: sfi/sfidl-parser.cc
===================================================================
--- sfi/sfidl-parser.cc	(revision 4138)
+++ sfi/sfidl-parser.cc	(working copy)
@@ -28,20 +28,30 @@
 #include <set>
 #include <stack>
 
-const std::string
-Sfidl::string_from_int (long long int lli)
+/* As opposed to the birnet function string_from_double, we use "%.17e"
+ * (instead "%.17g"). This keeps doubles in param specs like (50.0/100.0) in
+ * tact, because the C++ compiler will still recognize that we have doubles
+ * here. 
+ *
+ * [ "%g" omits decimal point and trailing digits for those doubles that can be
+ * represented without, so that the result of the division (when evaluated by
+ * the C++ compiler) would be 0, instead of the expected 0.5.]
+ */
+static std::string
+string_with_exponent_from_double (double value)
 {
-  gchar *cstr = g_strdup_printf ("%lld", lli);
-  std::string str = cstr;
-  g_free (cstr);
-  return str;
+  char buffer[G_ASCII_DTOSTR_BUF_SIZE + 1] = "";
+  g_ascii_formatd (buffer, G_ASCII_DTOSTR_BUF_SIZE, "%.17e", value);
+  return buffer;
 }
 
-
 namespace {
 using namespace Sfidl;
 using namespace std;
 
+using Birnet::string_from_uint;
+using Birnet::string_from_int;
+
 /* --- variables --- */
 static  GScannerConfig  scanner_config_template = {
   const_cast<gchar *>   /* FIXME: glib should use const gchar* here */
@@ -997,16 +1007,13 @@ GTokenType Parser::parseStringOrConst (s
 	  if (ci->name == s &&
               ci->type != Constant::tIdent)     /* identifier constants can't be expanded to strings */
 	    {
-	      char *x = 0;
 	      switch (ci->type)
 		{
 		  case Constant::tInt:
-		    s = x = g_strdup_printf ("%lldLL", ci->i);
-		    g_free (x);
+		    s = string_from_int (ci->i) + "LL";
 		    break;
 		  case Constant::tFloat:
-		    s = x = g_strdup_printf ("%.17g", ci->f);
-		    g_free (x);
+		    s = string_with_exponent_from_double (ci->f);
 		    break;
 		  case Constant::tString:
 		    s = ci->str;
@@ -1522,13 +1529,12 @@ GTokenType Parser::parseParamHints (Para
   while (!g_scanner_eof (scanner) && bracelevel > 0)
     {
       GTokenType t = scanner_get_next_token_with_colon_identifiers (scanner);
-      gchar *token_as_string = 0, *x = 0;
+      string token_as_string;
       bool current_arg_complete = false;
 
-      if(int(t) > 0 && int(t) <= 255)
+      if (int (t) > 0 && int (t) <= 255)
 	{
-	  token_as_string = g_new0 (char, 2); /* FIXME: leak */
-	  token_as_string[0] = char(t);
+	  token_as_string = char (t);
 	}
       switch (t)
 	{
@@ -1540,13 +1546,16 @@ GTokenType Parser::parseParamHints (Para
 	  break;
 	case ',':		  current_arg_complete = true;
 	  break;
-	case G_TOKEN_STRING:	  x = g_strescape (scanner->value.v_string, 0);
-				  token_as_string = g_strdup_printf ("\"%s\"", x);
-				  g_free (x);
+	case G_TOKEN_STRING:	  
+	  {
+	    char *tmp = g_strescape (scanner->value.v_string, 0);
+	    token_as_string = string ("\"") + tmp + "\"";
+	    g_free (tmp);
+	  }
 	  break;
-	case G_TOKEN_INT:	  token_as_string = g_strdup_printf ("%lluLL", scanner->value.v_int64);
+	case G_TOKEN_INT:	  token_as_string = string_from_uint (scanner->value.v_int64) + "LL";
 	  break;
-	case G_TOKEN_FLOAT:	  token_as_string = g_strdup_printf ("%.17g", scanner->value.v_float);
+	case G_TOKEN_FLOAT:	  token_as_string = string_with_exponent_from_double (scanner->value.v_float);
 	  break;
 	case G_TOKEN_IDENTIFIER:
           {
@@ -1562,22 +1571,24 @@ GTokenType Parser::parseParamHints (Para
 	      }
 
             if (ci == constants.end())
-              token_as_string = g_strdup_printf ("%s", scanner->value.v_identifier);
+              token_as_string = scanner->value.v_identifier;
             else switch (ci->type)
               {
               case Constant::tInt:
-                token_as_string = g_strdup_printf ("%lldLL", ci->i);
+                token_as_string = string_from_int (ci->i) + "LL";
                 break;
               case Constant::tFloat:
-                token_as_string = g_strdup_printf ("%.17g", ci->f);
+                token_as_string = string_with_exponent_from_double (ci->f);
                 break;
               case Constant::tIdent:
-                token_as_string = g_strdup (ci->str.c_str());
+                token_as_string = ci->str;
                 break;
               case Constant::tString:
-                x = g_strescape (ci->str.c_str(), 0);
-                token_as_string = g_strdup_printf ("\"%s\"", x);
-                g_free (x);
+	        {
+		  char *tmp = g_strescape (ci->str.c_str(), 0);
+		  token_as_string = string("\"") + tmp + "\"";
+		  g_free (tmp);
+		}
                 break;
               default:
                 g_assert_not_reached ();
@@ -1586,7 +1597,7 @@ GTokenType Parser::parseParamHints (Para
           }
 	  break;
 	default:
-	  if (!token_as_string)
+	  if (token_as_string.empty())
 	    return GTokenType (')');
 	}
       if (current_arg_complete)
@@ -1601,16 +1612,15 @@ GTokenType Parser::parseParamHints (Para
 	    }
 	  current_arg = "";
 	}
-      else if (token_as_string)
+      else if (!token_as_string.empty())
 	{
 	  current_arg += token_as_string;
 	}
-      if (token_as_string && bracelevel)
+      if (!token_as_string.empty() && bracelevel)
 	{
 	  if (args != "")
 	    args += ' ';
 	  args += token_as_string;
-	  g_free (token_as_string);
 	}
     }
   def.args = args;
Index: sfi/sfidl-corecxx.cc
===================================================================
--- sfi/sfidl-corecxx.cc	(revision 4138)
+++ sfi/sfidl-corecxx.cc	(working copy)
@@ -31,6 +31,8 @@ namespace {
 using namespace std;
 using namespace Sfidl;
 
+using Birnet::string_from_int;
+
 static const gchar*
 canonify_name (const string& s,
                const char    replace = '-')
Index: sfi/sfidl-parser.hh
===================================================================
--- sfi/sfidl-parser.hh	(revision 4138)
+++ sfi/sfidl-parser.hh	(working copy)
@@ -28,8 +28,6 @@
 
 namespace Sfidl {
 
-const std::string string_from_int (long long int lli);
-
 /* we implement a get() function since operator[] is not const */
 template<typename Key, typename Value>
 class Map : public std::map<Key,Value> {
Index: sfi/Makefile.am
===================================================================
--- sfi/Makefile.am	(revision 4138)
+++ sfi/Makefile.am	(working copy)
@@ -61,7 +61,7 @@ common_idl_sources = sfidl-generator.cc 
 
 bin_PROGRAMS = sfidl
 sfidl_SOURCES = sfidl.cc $(common_idl_sources)
-sfidl_LDADD = $(SFI_LIBS) -lm # libsfi.la
+sfidl_LDADD = $(SFI_LIBS) -lm $(top_builddir)/birnet/libbirnet.o # libsfi.la
 sfidl_CFLAGS = $(AM_CFLAGS) # hack to cause glib-extra.c to be compiled twice (work around automake)
 EXTRA_DIST += sfidl-generator.hh sfidl-namespace.hh sfidl-options.hh sfidl-parser.hh sfidl-factory.hh sfidl-typelist.hh
 EXTRA_DIST += sfidl-cbase.hh sfidl-clientc.hh sfidl-clientcxx.hh sfidl-corec.hh sfidl-corecxx.hh sfidl-cxxbase.hh sfidl-hostc.hh

It passes

   make all install report

Ok to commit?

   Cu... Stefan
-- 
Stefan Westerfeld, Hamburg/Germany, http://space.twc.de/~stefan