Cleanup during review of new pushed code

include/my_global.h:
  Safer macros to avoid possible overflows
sql/item_cmpfunc.cc:
  Simple optimization
sql/sp_head.cc:
  Indentation fixes
  Remove not needed "else" levels
  Added error checking for 'new'
  Simpler reseting of thd->spcont in execute_procedure
sql/sql_base.cc:
  Faster new
sql/sql_lex.cc:
  Use 'TRUE' instead of 'true'
sql/sql_parse.cc:
  Faster new
sql/sql_view.cc:
  No need to set 'tables' as it's not used
sql/table.cc:
  Simpler DBUG_ASSERT()

7 files changed, 141 insertions, 138 deletions

diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc
index 1bae5f1c9af..9443a2949d8 100644
--- a/sql/item_cmpfunc.cc
+++ b/sql/item_cmpfunc.cc
@@ -630,7 +630,7 @@ int Arg_comparator::compare_row()
       owner->null_value= 0;
       res= 0;  // continue comparison (maybe we will meet explicit difference)
     }
-    if (res)
+    else if (res)
       return res;
   }
   if (was_null)
@@ -645,6 +645,7 @@ int Arg_comparator::compare_row()
   return 0;
 }
 
+
 int Arg_comparator::compare_e_row()
 {
   (*a)->bring_value();
diff --git a/sql/sp_head.cc b/sql/sp_head.cc
index f119ef1ec22..ebcbfb67fc8 100644
--- a/sql/sp_head.cc
+++ b/sql/sp_head.cc
@@ -127,16 +127,17 @@ sp_prepare_func_item(THD* thd, Item **it_addr)
 
 
 /* Macro to switch arena in sp_eval_func_item */
-#define CREATE_ON_CALLERS_ARENA(new_command, condition, backup_arena) do\
-        {\
-          if (condition) \
-            thd->set_n_backup_item_arena(thd->spcont->callers_arena,\
-                                         backup_arena);\
-          new_command;\
-          if (condition)\
-            thd->restore_backup_item_arena(thd->spcont->callers_arena,\
-                                           &backup_current_arena);\
-        } while(0)
+#define CREATE_ON_CALLERS_ARENA(new_command, condition, backup_arena)   \
+  do                                                                    \
+  {                                                                     \
+    if (condition)                                                      \
+      thd->set_n_backup_item_arena(thd->spcont->callers_arena,          \
+                                   backup_arena);                       \
+    new_command;                                                        \
+    if (condition)                                                      \
+      thd->restore_backup_item_arena(thd->spcont->callers_arena,        \
+                                     &backup_current_arena);            \
+  } while(0)
 
 /*
   Evaluate an item and store it in the returned item
@@ -174,88 +175,82 @@ sp_eval_func_item(THD *thd, Item **it_addr, enum enum_field_types type,
     DBUG_RETURN(NULL);
   }
 
-  /* QQ How do we do this? Is there some better way? */
-  if (type == MYSQL_TYPE_NULL)
-    goto return_null_item;
-
   switch (sp_map_result_type(type)) {
   case INT_RESULT:
-    {
-      longlong i= it->val_int();
+  {
+    longlong i= it->val_int();
 
-      if (it->null_value)
-      {
-        DBUG_PRINT("info", ("INT_RESULT: null"));
-        goto return_null_item;
-      }
-      else
-      {
-        DBUG_PRINT("info", ("INT_RESULT: %d", i));
-        CREATE_ON_CALLERS_ARENA(it= new(reuse, &rsize) Item_int(i),
-                          use_callers_arena, &backup_current_arena);
-      }
-      break;
+    if (it->null_value)
+    {
+      DBUG_PRINT("info", ("INT_RESULT: null"));
+      goto return_null_item;
     }
+    DBUG_PRINT("info", ("INT_RESULT: %d", i));
+    CREATE_ON_CALLERS_ARENA(it= new(reuse, &rsize) Item_int(i),
+                            use_callers_arena, &backup_current_arena);
+    break;
+  }
   case REAL_RESULT:
-    {
-      double d= it->val_real();
+  {
+    double d= it->val_real();
+    uint8 decimals;
+    uint32 max_length;
 
-      if (it->null_value)
-      {
-        DBUG_PRINT("info", ("REAL_RESULT: null"));
-        goto return_null_item;
-      }
-      else
-      {
-        /* There's some difference between Item::new_item() and the
-         * constructor; the former crashes, the latter works... weird. */
-        uint8 decimals= it->decimals;
-        uint32 max_length= it->max_length;
-        DBUG_PRINT("info", ("REAL_RESULT: %g", d));
-        CREATE_ON_CALLERS_ARENA(it= new(reuse, &rsize) Item_float(d),
-                          use_callers_arena, &backup_current_arena);
-        it->decimals= decimals;
-        it->max_length= max_length;
-      }
-      break;
+    if (it->null_value)
+    {
+      DBUG_PRINT("info", ("REAL_RESULT: null"));
+      goto return_null_item;
     }
+
+    /*
+      There's some difference between Item::new_item() and the
+      constructor; the former crashes, the latter works... weird.
+    */
+    decimals= it->decimals;
+    max_length= it->max_length;
+    DBUG_PRINT("info", ("REAL_RESULT: %g", d));
+    CREATE_ON_CALLERS_ARENA(it= new(reuse, &rsize) Item_float(d),
+                            use_callers_arena, &backup_current_arena);
+    it->decimals= decimals;
+    it->max_length= max_length;
+    break;
+  }
   case DECIMAL_RESULT:
-    {
-      my_decimal value, *val= it->val_decimal(&value);
-      if (it->null_value)
-        goto return_null_item;
-      else
-        CREATE_ON_CALLERS_ARENA(it= new(reuse, &rsize) Item_decimal(val),
-                          use_callers_arena, &backup_current_arena);
+  {
+    my_decimal value, *val= it->val_decimal(&value);
+    if (it->null_value)
+      goto return_null_item;
+    CREATE_ON_CALLERS_ARENA(it= new(reuse, &rsize) Item_decimal(val),
+                            use_callers_arena, &backup_current_arena);
 #ifndef DBUG_OFF
+    {
       char dbug_buff[DECIMAL_MAX_STR_LENGTH+1];
-      DBUG_PRINT("info", ("DECIMAL_RESULT: %s", dbug_decimal_as_string(dbug_buff, val)));
+      DBUG_PRINT("info", ("DECIMAL_RESULT: %s",
+                          dbug_decimal_as_string(dbug_buff, val)));
 #endif
-      break;
     }
+    break;
+  }
   case STRING_RESULT:
-    {
-      char buffer[MAX_FIELD_WIDTH];
-      String tmp(buffer, sizeof(buffer), it->collation.collation);
-      String *s= it->val_str(&tmp);
+  {
+    char buffer[MAX_FIELD_WIDTH];
+    String tmp(buffer, sizeof(buffer), it->collation.collation);
+    String *s= it->val_str(&tmp);
 
-      if (it->null_value)
-      {
-        DBUG_PRINT("info", ("default result: null"));
-        goto return_null_item;
-      }
-      else
-      {
-        DBUG_PRINT("info",("default result: %*s",
-                           s->length(), s->c_ptr_quick()));
-        CREATE_ON_CALLERS_ARENA(it= new(reuse, &rsize)
-                              Item_string(thd->strmake(s->ptr(),
-                                          s->length()), s->length(),
-                                          it->collation.collation),
-                          use_callers_arena, &backup_current_arena);
-      }
-      break;
+    if (type == MYSQL_TYPE_NULL || it->null_value)
+    {
+      DBUG_PRINT("info", ("STRING_RESULT: null"));
+      goto return_null_item;
     }
+    DBUG_PRINT("info",("STRING_RESULT: %*s",
+                       s->length(), s->c_ptr_quick()));
+    CREATE_ON_CALLERS_ARENA(it= new(reuse, &rsize)
+                            Item_string(thd->strmake(s->ptr(),
+                                                     s->length()), s->length(),
+                                        it->collation.collation),
+                            use_callers_arena, &backup_current_arena);
+    break;
+  }
   case ROW_RESULT:
   default:
     DBUG_ASSERT(0);
@@ -574,13 +569,10 @@ sp_head::destroy()
 
 
 /*
- *  This is only used for result fields from functions (both during
- *  fix_length_and_dec() and evaluation).
- *
- *  Since the current mem_root during a will be freed and the result
- *  field will be used by the caller, we have to put it in the caller's
- *  or main mem_root.
- */
+  This is only used for result fields from functions (both during
+  fix_length_and_dec() and evaluation).
+*/
+
 Field *
 sp_head::make_field(uint max_length, const char *name, TABLE *dummy)
 {
@@ -817,47 +809,49 @@ sp_head::execute_function(THD *thd, Item **argp, uint argcount, Item **resp)
   sp_rcontext *octx = thd->spcont;
   sp_rcontext *nctx = NULL;
   uint i;
-  int ret;
+  Item_null *nit;
+  int ret= -1;                                  // Assume error
 
   if (argcount != params)
   {
     /*
-      Need to use my_printf_error here, or it will not terminate the
+      Need to use my_error here, or it will not terminate the
       invoking query properly.
     */
     my_error(ER_SP_WRONG_NO_OF_ARGS, MYF(0),
              "FUNCTION", m_qname.str, params, argcount);
-    DBUG_RETURN(-1);
+    goto end;
   }
 
 
   // QQ Should have some error checking here? (types, etc...)
-  nctx= new sp_rcontext(csize, hmax, cmax);
+  if (!(nctx= new sp_rcontext(csize, hmax, cmax)))
+    goto end;
   for (i= 0 ; i < argcount ; i++)
   {
     sp_pvar_t *pvar = m_pcont->find_pvar(i);
     Item *it= sp_eval_func_item(thd, argp++, pvar->type, NULL, FALSE);
 
-    if (it)
-      nctx->push_item(it);
-    else
-    {
-      DBUG_RETURN(-1);
-    }
+    if (!it)
+      goto end;                                 // EOM error
+    nctx->push_item(it);
   }
+
   /*
     The rest of the frame are local variables which are all IN.
     Default all variables to null (those with default clauses will
     be set by an set instruction).
   */
+
+  nit= NULL;                       // Re-use this, and only create if needed
+  for (; i < csize ; i++)
   {
-    Item_null *nit= NULL;	// Re-use this, and only create if needed
-    for (; i < csize ; i++)
+    if (! nit)
     {
-      if (! nit)
-	nit= new Item_null();
-      nctx->push_item(nit);
+      if (!(nit= new Item_null()))
+        DBUG_RETURN(-1);
     }
+    nctx->push_item(nit);
   }
   thd->spcont= nctx;
 
@@ -878,14 +872,15 @@ sp_head::execute_function(THD *thd, Item **argp, uint argcount, Item **resp)
   }
 
   nctx->pop_all_cursors();	// To avoid memory leaks after an error
-  delete nctx;
+  delete nctx;                                  // Doesn't do anything
   thd->spcont= octx;
 
+end:
   DBUG_RETURN(ret);
 }
 
-static Item_func_get_user_var *
-item_is_user_var(Item *it)
+
+static Item_func_get_user_var *item_is_user_var(Item *it)
 {
   if (it->type() == Item::FUNC_ITEM)
   {
@@ -897,19 +892,18 @@ item_is_user_var(Item *it)
   return NULL;
 }
 
-int
-sp_head::execute_procedure(THD *thd, List<Item> *args)
+
+int sp_head::execute_procedure(THD *thd, List<Item> *args)
 {
-  DBUG_ENTER("sp_head::execute_procedure");
-  DBUG_PRINT("info", ("procedure %s", m_name.str));
   int ret= 0;
   uint csize = m_pcont->max_pvars();
   uint params = m_pcont->current_pvars();
   uint hmax = m_pcont->max_handlers();
   uint cmax = m_pcont->max_cursors();
-  sp_rcontext *octx = thd->spcont;
+  sp_rcontext *save_spcont, *octx;
   sp_rcontext *nctx = NULL;
-  my_bool is_tmp_octx = FALSE;  // True if we have allocated a temporary octx
+  DBUG_ENTER("sp_head::execute_procedure");
+  DBUG_PRINT("info", ("procedure %s", m_name.str));
 
   if (args->elements != params)
   {
@@ -918,17 +912,22 @@ sp_head::execute_procedure(THD *thd, List<Item> *args)
     DBUG_RETURN(-1);
   }
 
+  save_spcont= octx= thd->spcont;
   if (! octx)
   {				// Create a temporary old context
-    octx= new sp_rcontext(csize, hmax, cmax);
-    is_tmp_octx= TRUE;
+    if (!(octx= new sp_rcontext(csize, hmax, cmax)))
+      DBUG_RETURN(-1);
     thd->spcont= octx;
 
     /* set callers_arena to thd, for upper-level function to work */
     thd->spcont->callers_arena= thd;
   }
 
-  nctx= new sp_rcontext(csize, hmax, cmax);
+  if (!(nctx= new sp_rcontext(csize, hmax, cmax)))
+  {
+    thd->spcont= save_spcont;
+    DBUG_RETURN(-1);
+  }
 
   if (csize > 0 || hmax > 0 || cmax > 0)
   {
@@ -937,7 +936,6 @@ sp_head::execute_procedure(THD *thd, List<Item> *args)
     List_iterator<Item> li(*args);
     Item *it;
 
-
     /* Evaluate SP arguments (i.e. get the values passed as parameters) */
     // QQ: Should do type checking?
     DBUG_PRINT("info",(" %.*s: eval args", m_name.length, m_name.str));
@@ -959,20 +957,25 @@ sp_head::execute_procedure(THD *thd, List<Item> *args)
 	if (pvar->mode == sp_param_out)
 	{
 	  if (! nit)
-	    nit= new Item_null();
+          {
+	    if (!(nit= new Item_null()))
+            {
+              ret= -1;
+              break;
+            }
+          }
 	  nctx->push_item(nit); // OUT
 	}
 	else
 	{
 	  Item *it2= sp_eval_func_item(thd, li.ref(), pvar->type, NULL, FALSE);
 
-	  if (it2)
-	    nctx->push_item(it2); // IN or INOUT
-	  else
+	  if (!it2)
 	  {
 	    ret= -1;		// Eval failed
 	    break;
 	  }
+          nctx->push_item(it2); // IN or INOUT
 	}
       }
     }
@@ -994,7 +997,13 @@ sp_head::execute_procedure(THD *thd, List<Item> *args)
     for (; i < csize ; i++)
     {
       if (! nit)
-	nit= new Item_null();
+      {
+	if (!(nit= new Item_null()))
+        {
+          ret= -1;
+          break;
+        }
+      }
       nctx->push_item(nit);
     }
   }
@@ -1090,15 +1099,12 @@ sp_head::execute_procedure(THD *thd, List<Item> *args)
     }
   }
 
-  if (is_tmp_octx)
-  {
-    delete octx;                                /* call destructor */
-    octx= NULL;
-  }
+  if (!save_spcont)
+    delete octx;                                // Does nothing
 
   nctx->pop_all_cursors();	// To avoid memory leaks after an error
-  delete nctx;
-  thd->spcont= octx;
+  delete nctx;                                  // Does nothing
+  thd->spcont= save_spcont;
 
   DBUG_RETURN(ret);
 }
diff --git a/sql/sql_base.cc b/sql/sql_base.cc
index 23b8dae24fb..186d5984dcd 100644
--- a/sql/sql_base.cc
+++ b/sql/sql_base.cc
@@ -3363,7 +3363,7 @@ static bool
 set_new_item_local_context(THD *thd, Item_ident *item, TABLE_LIST *table_ref)
 {
   Name_resolution_context *context;
-  if (!(context= new Name_resolution_context))
+  if (!(context= new (thd->mem_root) Name_resolution_context))
     return TRUE;
   context->init();
   context->first_name_resolution_table=
diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc
index 6ede870a7fb..031d133a40c 100644
--- a/sql/sql_lex.cc
+++ b/sql/sql_lex.cc
@@ -1520,7 +1520,7 @@ void st_select_lex_unit::print(String *str)
       if (union_all)
 	str->append("all ", 4);
       else if (union_distinct == sl)
-        union_all= true;
+        union_all= TRUE;
     }
     if (sl->braces)
       str->append('(');
diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
index 619a84e185e..31d0f3eb675 100644
--- a/sql/sql_parse.cc
+++ b/sql/sql_parse.cc
@@ -6420,7 +6420,7 @@ Name_resolution_context *
 make_join_on_context(THD *thd, TABLE_LIST *left_op, TABLE_LIST *right_op)
 {
   Name_resolution_context *on_context;
-  if (!(on_context= new Name_resolution_context))
+  if (!(on_context= new (thd->mem_root) Name_resolution_context))
     return NULL;
   on_context->init();
   on_context->first_name_resolution_table=
diff --git a/sql/sql_view.cc b/sql/sql_view.cc
index af21b43e5c9..dcada0c0780 100644
--- a/sql/sql_view.cc
+++ b/sql/sql_view.cc
@@ -310,7 +310,7 @@ bool mysql_create_view(THD *thd,
     open_and_lock_tables can change the value of tables,
     e.g. it may happen if before the function call tables was equal to 0. 
   */ 
-  for (tbl= tables= lex->query_tables; tbl; tbl= tbl->next_global)
+  for (tbl= lex->query_tables; tbl; tbl= tbl->next_global)
   {
     /* is this table temporary and is not view? */
     if (tbl->table->s->tmp_table != NO_TMP_TABLE && !tbl->view &&
diff --git a/sql/table.cc b/sql/table.cc
index beecd6442e8..e84f8adc32f 100644
--- a/sql/table.cc
+++ b/sql/table.cc
@@ -2518,12 +2518,9 @@ void Field_iterator_natural_join::set(TABLE_LIST *table_ref)
 void Field_iterator_natural_join::next()
 {
   cur_column_ref= (*column_ref_it)++;
-  DBUG_ASSERT(cur_column_ref ?
-              (cur_column_ref->table_field ?
-               cur_column_ref->table_ref->table ==
-               cur_column_ref->table_field->table :
-               TRUE) :
-              TRUE);
+  DBUG_ASSERT(!cur_column_ref || ! cur_column_ref->table_field ||
+              cur_column_ref->table_ref->table ==
+              cur_column_ref->table_field->table);
 }
 
 
@@ -2695,9 +2692,8 @@ Field_iterator_table_ref::get_or_create_column_ref(THD *thd, bool *is_created)
     nj_col= natural_join_it.column_ref();
     DBUG_ASSERT(nj_col);
   }
-  DBUG_ASSERT(nj_col->table_field ?
-              nj_col->table_ref->table == nj_col->table_field->table :
-              TRUE);
+  DBUG_ASSERT(!nj_col->table_field ||
+              nj_col->table_ref->table == nj_col->table_field->table);
   return nj_col;
 }