Merge lp:~posulliv/drizzle/cleanup-replace-typelib into lp:~drizzle-trunk/drizzle/development

[Padraig O'Sullivan]

[Padraig O'Sullivan (posulliv)]

This patch replaces an instance of TYPELIB with std::vector. I looked for a simple, self-contained instance of TYPELIB to start with and I found one in client/drizzleadmin.cc. I wanted to start on a small and simple instance like this one so that I can get feedback on whether I am going about this the right way or not. I'm hoping to work on replacing more instances of TYPELIB throughout the code in the next few weeks.

I'm looking to get comments on whether what I did to replace TYPELIB in this case is the best way to go about it. I wasn't sure if I should have gone with std::map or std::vector in this case. I found it easier to go with std::vector so that was my main motivation for using it here.

[lp:~posulliv/drizzle/cleanup-replace-typelib]

959. By Brian Aker on 2009-03-20

Merge from Stewart.

960. By Brian Aker on 2009-03-20

Merging Monty

[Jay Pipes (jaypipes)]

Looks good to me!

review: Approve

[Monty Taylor (mordred)]

Padraig O'Sullivan wrote:
> Padraig O'Sullivan has proposed merging
> lp:~posulliv/drizzle/cleanup-replace-typelib into lp:drizzle.
>
> Requested reviews: Drizzle-developers (drizzle-developers)
>
> This patch replaces an instance of TYPELIB with std::vector. I looked
> for a simple, self-contained instance of TYPELIB to start with and I
> found one in client/drizzleadmin.cc. I wanted to start on a small and
> simple instance like this one so that I can get feedback on whether I
> am going about this the right way or not. I'm hoping to work on
> replacing more instances of TYPELIB throughout the code in the next
> few weeks.
>
> I'm looking to get comments on whether what I did to replace TYPELIB
> in this case is the best way to go about it. I wasn't sure if I
> should have gone with std::map or std::vector in this case. I found
> it easier to go with std::vector so that was my main motivation for
> using it here.
>

Looks good in general... but now I'm going to nitpick... :)

If I were you, I'd make command_names a string[] and command_vector a
vector of std::string. Then you can get rid of CommandMatch and just do:

static int get_command_type(const char *name)
{
  int type= ADMIN_ERROR;
  string comp_string= lower_string(name);
  vector<string>::iterator it=
    std::find(command_vector.begin(), command_vector.end(),
              comp_string);
  if (it != command_vector.end())
  {
    /* add 1 due to the way the commands ENUM is defined */
    type= distance(command_vector.begin(), it) + 1;
  }
  return type;
}

Monty

[Padraig O'Sullivan (posulliv)]

Yep, good point. I updated the branch and was able to remove the function object like you mentioned. Ran the test suite also and its all good so I pushed the changes to the branch.

Thanks guys for the comments! Btw, I created a blueprint for the task of replacing TYPELIB and linked it to the code-cleanup-c++ blueprint. I also added the TYPELIB blueprint to the low-hanging fruit milestone in case anyone else wants to work on it before I get started with it.

-Padraig

[lp:~posulliv/drizzle/cleanup-replace-typelib]

961. By Brian Aker on 2009-03-22

Merge Monty

962. By Brian Aker on 2009-03-23

Merge in Brian's work on processlist locks.

963. By Brian Aker on 2009-03-25

Merge in factoring out sql_plugin work.

964. By Brian Aker on 2009-03-25

Merge of Monty

965. By Brian Aker on 2009-03-25

Merge with Jay

966. By Brian Aker on 2009-03-26

Merge Stewart.

967. By Brian Aker on 2009-03-27

Merge Brian

968. By Brian Aker on 2009-03-27

Merge from Brian

969. By Brian Aker on 2009-03-30

Merge of Eric's libdrizzle work.

970. By Brian Aker on 2009-03-30

Merge Monty.

971. By Brian Aker on 2009-04-01

Merge Monty

=== modified file 'client/drizzleadmin.cc'
--- client/drizzleadmin.cc	2009-03-04 22:49:53 +0000
+++ client/drizzleadmin.cc	2009-03-20 21:51:00 +0000
@@ -24,12 +24,16 @@
 #include <signal.h>
 #include <mysys/my_pthread.h>				/* because of signal()	*/
 #include <sys/stat.h>
+#include <vector>
+#include <algorithm>
+#include <string>
 
 /* Added this for string translation. */
 #include <drizzled/gettext.h>
 
 #define ADMIN_VERSION "8.42"
 #define SHUTDOWN_DEF_TIMEOUT 3600		/* Wait for shutdown */
+#define NUM_COMMAND_NAMES 2
 
 char *host= NULL, *user= NULL, *opt_password= NULL;
 static bool interrupted= false, opt_verbose= false,tty_password= false;
@@ -62,12 +66,11 @@
 
 static const char *command_names[]= {
   "shutdown",
-  "ping",
-  NULL
+  "ping"
 };
 
-static TYPELIB command_typelib=
-{ array_elements(command_names)-1,"commands", command_names, NULL };
+static vector<const char *> 
+  command_vector(command_names, command_names + NUM_COMMAND_NAMES);
 
 static struct my_option my_long_options[] =
 {
@@ -106,12 +109,65 @@
 
 static const char *load_default_groups[]= { "drizzleadmin","client",0 };
 
+inline string lower_string(const char * from_string)
+{
+  string to_string= from_string;
+  transform(to_string.begin(), to_string.end(),
+            to_string.begin(), ::tolower);
+  return to_string;
+}
+
+/*
+ * Function object to be used to compare a string
+ * against the vector of command strings. This function
+ * object is used in the get_command_type() method.
+ */
+template <class T>
+class CommandMatch :
+  public unary_function<const string&, bool>
+{
+  string match_text;
+  T match_func;
+  public:
+  CommandMatch(string text) : match_text(text) { }
+  inline bool operator()(const string match_against) const
+  {
+    return match_func(match_against, match_text);
+  }
+};
+
+/*
+ * Searches for the given command and determines
+ * its type.
+ *
+ * Returns the type of the command corresponding
+ * to the commands enum defined in drizzleadmin.cc
+ * If the command is not supported, return ADMIN_ERROR.
+ *
+ * @param command name to search for
+ */
+static int get_command_type(const char *name)
+{
+  int type= ADMIN_ERROR;
+  string comp_string= lower_string(name);
+  vector<const char *>::iterator it= 
+    std::find_if(command_vector.begin(),
+                 command_vector.end(),
+                 CommandMatch< equal_to<string> >(comp_string));
+  if (it != command_vector.end())
+  {
+    /* add 1 due to the way the commands ENUM is defined */
+    type= distance(command_vector.begin(), it) + 1;
+  }
+  return type;
+}
+
 bool
 get_one_option(int optid, const struct my_option *, char *argument)
 {
   char *endchar= NULL;
   uint64_t temp_drizzle_port= 0;
-  int error = 0;
+  int error= 0;
 
   switch(optid) {
   case 'p':
@@ -138,7 +194,7 @@
   case 'P':
     if (argument)
     {
-      char *start=argument;
+      char *start= argument;
       if (opt_password)
         free(opt_password);
 
@@ -174,7 +230,7 @@
     if (argument)
     {
       if ((option_wait=atoi(argument)) <= 0)
-        option_wait=1;
+        option_wait= 1;
     }
     else
       option_wait= ~(uint32_t)0;
@@ -202,7 +258,7 @@
   MY_INIT(argv[0]);
   drizzleclient_create(&drizzle);
   load_defaults("drizzle",load_default_groups,&argc,&argv);
-  save_argv = argv;				/* Save for free_defaults */
+  save_argv= argv;				/* Save for free_defaults */
   if ((ho_error=handle_options(&argc, &argv, my_long_options, get_one_option)))
   {
     free_defaults(save_argv);
@@ -215,7 +271,7 @@
     exit(1);
   }
 
-  commands = argv;
+  commands= argv;
   if (tty_password)
     opt_password = drizzleclient_get_tty_password(NULL);
 
@@ -224,7 +280,7 @@
 
   if (opt_connect_timeout)
   {
-    uint32_t tmp=opt_connect_timeout;
+    uint32_t tmp= opt_connect_timeout;
     drizzleclient_options(&drizzle,DRIZZLE_OPT_CONNECT_TIMEOUT, (char*) &tmp);
   }
 
@@ -238,7 +294,8 @@
       /* Return 0 if all commands are PING */
       for (; argc > 0; argv++, argc--)
       {
-        if (find_type(argv[0], &command_typelib, 2) != ADMIN_PING)
+        int type= get_command_type(argv[0]);
+        if (type != ADMIN_PING)
         {
           error= 1;
           break;
@@ -248,7 +305,7 @@
   }
   else
   {
-    error=execute_commands(&drizzle,argc,commands);
+    error= execute_commands(&drizzle,argc,commands);
     drizzleclient_close(&drizzle);
   }
   free(opt_password);
@@ -260,12 +317,12 @@
 
 void endprog(int)
 {
-  interrupted=1;
+  interrupted= 1;
 }
 
 static bool sql_connect(DRIZZLE *drizzle, uint32_t wait)
 {
-  bool info=0;
+  bool info= 0;
 
   for (;;)
   {
@@ -313,7 +370,7 @@
     {
       if (!info)
       {
-        info=1;
+        info= 1;
         fputs(_("Waiting for Drizzle server to answer"),stderr);
         (void) fflush(stderr);
       }
@@ -343,7 +400,8 @@
   */
   for (; argc > 0 ; argv++,argc--)
   {
-    switch (find_type(argv[0],&command_typelib,2)) {
+    int type= get_command_type(argv[0]);
+    switch (type) {
     case ADMIN_SHUTDOWN:
     {
       if (opt_verbose)
@@ -360,11 +418,11 @@
       if (opt_verbose)
         printf(_("done\n"));
 
-      argc=1;             /* Force SHUTDOWN to be the last command */
+      argc= 1;             /* Force SHUTDOWN to be the last command */
       break;
     }
     case ADMIN_PING:
-      drizzle->reconnect=0;	/* We want to know of reconnects */
+      drizzle->reconnect= 0;	/* We want to know of reconnects */
       if (!drizzleclient_ping(drizzle))
       {
         if (option_silent < 2)
@@ -374,7 +432,7 @@
       {
         if (drizzleclient_errno(drizzle) == CR_SERVER_GONE_ERROR)
         {
-          drizzle->reconnect=1;
+          drizzle->reconnect= 1;
           if (!drizzleclient_ping(drizzle))
             puts(_("connection was down, but drizzled is now alive"));
         }
@@ -385,7 +443,7 @@
           return -1;
         }
       }
-      drizzle->reconnect=1;	/* Automatic reconnect is default */
+      drizzle->reconnect= 1;	/* Automatic reconnect is default */
       break;
 
     default: