List:Commits« Previous MessageNext Message »
From:Tor Didriksen Date:January 11 2011 8:15am
Subject:bzr commit into mysql-next-mr-bugfixing branch (tor.didriksen:3243) WL#5257
View as plain text  
#At file:///export/home/didrik/repo/next-mr-opt-backporting-wl4800/ based on revid:jorgen.loland@stripped21073433-nqg9a60zdwbb0tmy

 3243 Tor Didriksen	2011-01-11
      WL#5257 Review comments.
      
      More comments in followup email ...
     @ sql/opt_range.cc
        
     @ sql/opt_trace.cc
        

    added:
      unittest/gunit/opt_notrace-t.cc
    modified:
      sql/opt_range.cc
      sql/opt_trace.cc
      sql/opt_trace.h
      sql/opt_trace2server.cc
      sql/sys_vars.cc
      unittest/gunit/CMakeLists.txt
      unittest/gunit/opt_trace-t.cc
=== modified file 'sql/opt_range.cc'
--- a/sql/opt_range.cc	2010-12-21 07:34:33 +0000
+++ b/sql/opt_range.cc	2011-01-11 08:15:46 +0000
@@ -793,8 +793,10 @@ static
 TRP_GROUP_MIN_MAX *get_best_group_min_max(PARAM *param, SEL_TREE *tree,
                                           double read_time);
 #if !defined(DBUG_OFF) || defined(OPTIMIZER_TRACE)
+// print_sel_tree declared 'static' but never defined
 static void print_sel_tree(PARAM *param, SEL_TREE *tree, key_map *tree_map,
                            const char *msg);
+// print_ror_scans_arr declared 'static' but never defined
 static void print_ror_scans_arr(TABLE *table, const char *msg,
                                 struct st_ror_scan_info **start,
                                 struct st_ror_scan_info **end);
@@ -12393,6 +12395,7 @@ print_multiple_key_values(KEY_PART *key_
   dbug_tmp_restore_column_maps(table->read_set, table->write_set, old_sets);
 }
 
+// print_quick defined but not used.
 static void print_quick(QUICK_SELECT_I *quick, const key_map *needed_reg)
 {
   char buf[MAX_KEY/8+1];

=== modified file 'sql/opt_trace.cc'
--- a/sql/opt_trace.cc	2010-11-24 18:54:26 +0000
+++ b/sql/opt_trace.cc	2011-01-11 08:15:46 +0000
@@ -65,7 +65,7 @@ void Opt_trace_struct::do_construct(Opt_
 {
   saved_key= key;
   requires_key= requires_key_arg;
-  /* DBUG_PRINT supports NULL in %s */
+
   DBUG_PRINT("opt_trace", ("%s: starting %s", key, types[requires_key]));
   stmt= stmt_arg;
   parent= parent_arg;
@@ -74,8 +74,7 @@ void Opt_trace_struct::do_construct(Opt_
 #ifndef DBUG_OFF
   previous_key[0]= 0;
 #endif
-  save_stmt_support_I_S=
-    stmt->support_I_S;
+  save_stmt_support_I_S= stmt->support_I_S;
   if (save_stmt_support_I_S &&
       (feature & stmt->ctx->features) == 0)
   {
@@ -122,9 +121,9 @@ void Opt_trace_struct::do_construct(Opt_
       if (max_size >= safety_margin)
       {
         max_size-= safety_margin;
-        if (new_size > max_size) // don't pre-allocate more than the limit
+        if (new_size > max_size) // Don't pre-allocate more than the limit.
           new_size= max_size;
-        if (new_size >= alloced) // never shrink string
+        if (new_size >= alloced) // Never shrink string.
           stmt->buffer.realloc(new_size);
       }
     }
@@ -141,7 +140,7 @@ void Opt_trace_struct::do_construct(Opt_
 
 void Opt_trace_struct::add_key_name(const char *key)
 {
-  /* user should always add to the innermost open object, not outside */
+  // User should always add to the innermost open object, not outside.
   DBUG_ASSERT(stmt->current_struct == this);
   bool has_key= key != NULL;
   if (unlikely(has_key != requires_key))
@@ -178,18 +177,18 @@ void Opt_trace_struct::add_struct(const 
 }
 
 
-void Opt_trace_struct::do_destruct(void)
+void Opt_trace_struct::do_destruct()
 {
   DBUG_PRINT("opt_trace", ("%s: ending %s", saved_key, types[requires_key]));
   DBUG_ASSERT(started);
   /*
     Note:
     - the Server code invokes Opt_trace_struct's constructor/destructor, and so
-    stmt's current_struct is necessarily set by Opt_trace_struct's
-    constructor/destructor
+      stmt's current_struct is necessarily set by Opt_trace_struct's
+      constructor/destructor
     - the Server code invokes Opt_trace_context start/end() functions, and so
-    context's current_stmt_in_gen is set in start()/end() (which is clean: object
-    sets _its_ variables, does not let its child do it).
+      context's current_stmt_in_gen is set in start()/end() (which is clean:
+      object sets _its_ variables, does not let its child do it).
   */
   stmt->current_struct= parent;
   DBUG_ASSERT(parent == NULL || stmt == parent->stmt);
@@ -236,7 +235,7 @@ Opt_trace_struct& Opt_trace_struct::do_a
       ("select" and other language keywords). If we directly store that mix in a
       UTF8 column, the query_charset piece causes an issue:
       Field_blob::store() will truncate the trace at first unrecognized
-      character. At worse, a single bad character in the expanded query makes
+      character. Even worse, a single bad character in the expanded query makes
       all the rest of the trace be lost.
       To work around the bug, we force a conversion from UTF8 to UTF8, which
       will replace any non-UTF8 characters with '?'. Some query_charset
@@ -250,7 +249,6 @@ Opt_trace_struct& Opt_trace_struct::do_a
       stmt->buffer.append_escaped(val, val_length);
     else
     {
-
       uint32 new_length= system_charset_info->mbmaxlen * val_length;
       char *utf8_str= (char *)my_malloc(new_length, MYF(0));
       if (utf8_str == NULL)
@@ -281,6 +279,10 @@ Opt_trace_struct& Opt_trace_struct::do_a
   return *this;
 }
 
+namespace {
+LEX_CSTRING readables[]= { { STRING_WITH_LEN("false") },
+                           { STRING_WITH_LEN("true") } };
+}
 
 Opt_trace_struct& Opt_trace_struct::do_add(const char *key, bool val)
 {
@@ -290,8 +292,6 @@ Opt_trace_struct& Opt_trace_struct::do_a
     return *this;
   stmt->separator();
   add_key_name(key);
-  LEX_CSTRING readables[]= { { STRING_WITH_LEN("false") },
-                             { STRING_WITH_LEN("true") } };
   const LEX_CSTRING *readable= &readables[(int)val];
   stmt->buffer.append(readable->str, readable->length);
   return *this;
@@ -301,7 +301,7 @@ Opt_trace_struct& Opt_trace_struct::do_a
 Opt_trace_struct& Opt_trace_struct::do_add(const char *key, longlong val)
 {
   DBUG_ASSERT(started);
-  char buf[22];
+  char buf[22];                                 // magic constant
   llstr(val, buf);
   DBUG_PRINT("opt_trace", ("%s: %s", key, buf));
   if (!stmt->support_I_S)
@@ -331,7 +331,7 @@ Opt_trace_struct& Opt_trace_struct::do_a
 Opt_trace_struct& Opt_trace_struct::do_add(const char *key, double val)
 {
   DBUG_ASSERT(started);
-  char buf[32];
+  char buf[32];                                 // magic constant
   my_snprintf(buf, sizeof(buf), "%g", val);
   DBUG_PRINT("opt_trace", ("%s: %s", key, buf));
   if (!stmt->support_I_S)
@@ -384,17 +384,18 @@ const char *Opt_trace_context::feature_n
   "greedy_search", "range_optimizer", "default", NullS
 };
 
-const Opt_trace_context::feature_value Opt_trace_context::FEATURES_DEFAULT=
+const Opt_trace_context::feature_value 
+Opt_trace_context::default_features=
   Opt_trace_context::feature_value(Opt_trace_context::GREEDY_SEARCH |
                                    Opt_trace_context::RANGE_OPTIMIZER);
 
-Opt_trace_context::Opt_trace_context(void):
+Opt_trace_context::Opt_trace_context():
   oldest_stmt_to_show(NULL), newest_stmt_to_show(NULL), stmt_to_del(NULL),
   since_offset_0(0), current_stmt_in_gen(NULL), cannot_change_settings(false)
 {}
 
 
-Opt_trace_context::~Opt_trace_context(void)
+Opt_trace_context::~Opt_trace_context()
 {
   /* There may well be some few ended traces left: */
   purge(true);
@@ -405,7 +406,7 @@ Opt_trace_context::~Opt_trace_context(vo
 }
 
 
-Opt_trace_struct *Opt_trace_context::get_current_struct(void) const
+Opt_trace_struct *Opt_trace_context::get_current_struct() const
 {
   return current_stmt_in_gen->current_struct;
 }
@@ -476,13 +477,13 @@ void Opt_trace_context::purge(bool purge
     /* This case is managed in @c Opt_trace_context::start() */
     DBUG_VOID_RETURN;
   }
-  ulong i= 0; // distance to the newest trace (unit: 'list elements')
+  long i= 0; // distance to the newest trace (unit: 'list elements')
   Opt_trace_stmt *stmt;
   /* Start from the newest traces, scroll back in time */
   for (stmt= newest_stmt_to_show ; stmt != NULL ; )
   {
     i++;
-    if (!purge_all && i <= (ulong)(-offset))
+    if (!purge_all && i <= (-offset))
     {
       /* OFFSET mandates that this trace should be kept; move to previous */
       stmt= stmt->prev;
@@ -606,6 +607,7 @@ bool Opt_trace_context::start(bool suppo
     {
       DBUG_PRINT("opt_trace", ("disabled: since_offset_0(%llu) >="
                                " offset(%lu) + limit(%lu)",
+                               // Both offset and limit are signed.
                                (ulonglong)since_offset_0, offset, limit));
       new_stmt_support_I_S= false;
     }
@@ -616,6 +618,7 @@ bool Opt_trace_context::start(bool suppo
                                            new_stmt_support_I_S);
   DBUG_PRINT("opt_trace",("new stmt %p support_I_S %d", stmt,
                           (int)new_stmt_support_I_S));
+  // new cannot return NULL.
   if (stmt == NULL)
     DBUG_RETURN(true);
 
@@ -645,7 +648,7 @@ bool Opt_trace_context::start(bool suppo
 }
 
 
-void Opt_trace_context::end(void)
+void Opt_trace_context::end()
 {
   if (current_stmt_in_gen != NULL)
   {
@@ -683,26 +686,30 @@ void Opt_trace_context::end(void)
 }
 
 
-size_t Opt_trace_context::allowed_mem_size(const Opt_trace_stmt *for_stmt)
-  const
+namespace {
+size_t allowed_mem_size(const Opt_trace_stmt *stmt_start,
+                        const Opt_trace_stmt *for_stmt)
 {
-  DBUG_ENTER("Opt_trace_context::allowed_mem_size");
   size_t mem_size= 0;
-  /* Even to-be-deleted traces use memory, so consider them in sum */
-  for (Opt_trace_stmt *stmt_start= newest_stmt_to_show ;
-       ;
-       stmt_start= stmt_to_del)
+  for (const Opt_trace_stmt *stmt= stmt_start ;
+       stmt != NULL ; stmt= stmt->prev_())
   {
-    for (Opt_trace_stmt *stmt= stmt_start ; stmt != NULL ; )
-    {
-      if (stmt != for_stmt)
-        mem_size+= stmt->buffer.alloced_length() +
-          stmt->query_buffer.alloced_length();
-      stmt= stmt->prev;
-    }
-    if (stmt_start == stmt_to_del)
-      break;
+    if (stmt != for_stmt)
+      mem_size+= stmt->alloced_length();
   }
+  return mem_size;
+}
+}
+
+
+size_t Opt_trace_context::allowed_mem_size(const Opt_trace_stmt *for_stmt) const
+{
+  DBUG_ENTER("Opt_trace_context::allowed_mem_size");
+  /* Even to-be-deleted traces use memory, so consider them in sum */
+  size_t mem_size= 
+    ::allowed_mem_size(newest_stmt_to_show, for_stmt) +
+    ::allowed_mem_size(stmt_to_del, for_stmt);
+
   size_t rc= (mem_size <= max_mem_size) ? (max_mem_size - mem_size) : 0;
   DBUG_PRINT("opt_trace", ("rc %llu max_mem_size %llu",
                            (ulonglong)rc, (ulonglong)max_mem_size));
@@ -723,7 +730,7 @@ const char *Opt_trace_context::get_tail(
 }
 
 
-void Opt_trace_context::reset(void)
+void Opt_trace_context::reset()
 {
   purge(true);
   since_offset_0= 0;
@@ -750,20 +757,20 @@ void Opt_trace_iterator::operator++(int)
       'limit'; if offset>=0, we stopped producing traces when 'limit' was
       reached.
     */
-    cursor= NULL; // like the 'end' iterator has
+    cursor= NULL;
   }
   row_count++;
 }
 
 
 /**
-   @note because allocation is done in big chunks, buffer->Ptr[str_length]
+   @note Because allocation is done in big chunks, buffer->Ptr[str_length]
    may be uninitialized while buffer->Ptr[allocated length] is 0, so we
    must use c_ptr_safe() as we want a NUL-terminated string (which is easier
    to manipulate in a debugger, or to compare in unit tests with
-   ASSERT_STREQ).
+   EXPECT_STREQ).
    c_ptr_safe() may realloc an empty String from 0 bytes to 8 bytes,
-   when it adds the closing NUL.
+   when it adds the closing NULL.
 */
 Opt_trace_info Opt_trace_iterator::operator*()
 {
@@ -780,12 +787,9 @@ Opt_trace_info Opt_trace_iterator::opera
 }
 
 
-Opt_trace_iterator Opt_trace_iterator::end;
-
-
 /* Opt_trace_stmt class */
 
-const size_t Opt_trace_stmt::empty_trace_len= 4;
+const size_t Opt_trace_stmt::empty_trace_len= 4; // magic number
 const size_t Opt_trace_stmt::buffer_alloc_increment= 1024;
 
 
@@ -803,7 +807,7 @@ Opt_trace_stmt::Opt_trace_stmt(Opt_trace
 }
 
 
-void Opt_trace_stmt::end(void)
+void Opt_trace_stmt::end()
 {
   DBUG_ASSERT(depth == 0);
   depth= 0;
@@ -825,22 +829,30 @@ void Opt_trace_stmt::end(void)
 }
 
 
-void Opt_trace_stmt::next_line(void)
+static const char my_spaces[] =
+  "                                                                "
+  "                                                                "
+  "                                                                "
+  "                                                                "
+  "                                                                "
+  "                                                                "
+  "                                                                "
+  ; // Extend as desired, to max supported indentation level.
+void Opt_trace_stmt::next_line()
 {
   if (ctx->one_line)
     return;
   buffer.append('\n');
-  char spaces[]= "                               ";
-  // 2 spaces per nesting level
-  const int nb= depth * 2, spaces_len= sizeof(spaces) - 1;
-  // add spaces in chunks of spaces_len, better than many append(' ')
-  for (int i= 0; i < nb/spaces_len; i++)
-    buffer.append(spaces, spaces_len);
-  buffer.append(spaces, (nb % spaces_len));
+
+  // You can get away with a single append here:
+  int ix= sizeof(my_spaces) - 2*depth - 1;
+  if (ix < 0)
+    ix= 0;
+  buffer.append(&my_spaces[ix]);
 }
 
 
-void Opt_trace_stmt::push(void)
+void Opt_trace_stmt::push()
 {
   DBUG_ASSERT(support_I_S);
   depth++;
@@ -854,19 +866,16 @@ void Opt_trace_stmt::push(void)
 }
 
 
-void Opt_trace_stmt::pop(void)
+void Opt_trace_stmt::pop()
 {
   DBUG_ASSERT(support_I_S);
   depth--;
 }
 
 
-void Opt_trace_stmt::separator(void)
+void Opt_trace_stmt::separator()
 {
-  /*
-    If we've already written an object at this level, first put a
-    comma.
-  */
+  // Put a comma first, if we have already written an object at this level.
   if (!level_empty.at(depth))
     buffer.append(',');
   next_line();
@@ -877,19 +886,19 @@ void Opt_trace_stmt::separator(void)
 bool Opt_trace_stmt::set_query(const char *query, size_t length,
                                CHARSET_INFO *charset)
 {
-  /* Should be called only once per statement */
+  // Should be called only once per statement.
   DBUG_ASSERT(query_buffer.ptr() == NULL);
   query_buffer.set_charset(charset);
-  /* We're taking a bit of space from 'buffer' */
+  // We are taking a bit of space from 'buffer'.
   if (buffer.alloced_length() >= buffer.allowed_mem_size)
   {
-    /* trace buffer already occupies all space */
+    // Trace buffer already occupies all space.
     buffer.missing_bytes+= length;
     return true;
   }
   const size_t available= buffer.allowed_mem_size - buffer.alloced_length();
   query_buffer.allowed_mem_size= available;
-  /* No need to escape query, this is not for JSON */
+  /* No need to escape query, this is not for JSON. */
   const bool rc= query_buffer.append(query, length);
   /* Space which query took is taken out of the trace: */
   if (query_buffer.alloced_length() >= buffer.allowed_mem_size)
@@ -904,7 +913,7 @@ bool Opt_trace_stmt::set_query(const cha
 
 bool Opt_trace_stmt::Buffer::realloc(uint32 arg_length)
 {
-  bool rc= String::realloc(arg_length);
+  bool rc= string_buf.realloc(arg_length);
   malloc_error|= rc;
   return rc;
 }
@@ -931,7 +940,8 @@ bool Opt_trace_stmt::Buffer::append_esca
     {
       rc= false;
       const char *pstr, *pstr_end;
-      char buf[128] /* temporary output buffer*/, *pbuf= buf;
+      char buf[128];                     // Temporary output buffer.
+      char *pbuf= buf;
       for (pstr= str, pstr_end= (str + length) ; pstr < pstr_end ; pstr++)
       {
         char esc;
@@ -939,8 +949,8 @@ bool Opt_trace_stmt::Buffer::append_esca
         /*
           JSON syntax says that control characters must be escaped. Experience
           confirms that this means ASCII 0->31 and " and \ . A few of
-          them are accepted with a short escaping syntax (using \ : like
-          \n) but for most of them, only \uXXXX works, where XXXX is an
+          them are accepted with a short escaping syntax (using \ : like \n)
+          but for most of them, only \uXXXX works, where XXXX is a
           hexadecimal value for the code point.
           Rules also mention escaping / , but Python's and Perl's json modules
           do not require it, and somewhere on Internet someone said JSON
@@ -948,7 +958,7 @@ bool Opt_trace_stmt::Buffer::append_esca
         */
         switch (c)
         {
-          // don't use \u when possible for common chars, \ is easier to read:
+          // Don't use \u when possible for common chars, \ is easier to read:
         case '\\':
           esc= '\\';
           break;
@@ -968,7 +978,7 @@ bool Opt_trace_stmt::Buffer::append_esca
           esc= 0;
           break;
         }
-        if (esc != 0)                           // escaping with backslash
+        if (esc != 0)                           // Escaping with backslash.
         {
           *pbuf++= '\\';
           *pbuf++= esc;
@@ -976,7 +986,7 @@ bool Opt_trace_stmt::Buffer::append_esca
         else
         {
           uint ascii_code= (uint)c;
-          if (ascii_code < 32)                  // escaping with \u
+          if (ascii_code < 32)                  // Escaping with \u
           {
             *pbuf++= '\\';
             *pbuf++= 'u';
@@ -994,17 +1004,17 @@ bool Opt_trace_stmt::Buffer::append_esca
             *pbuf++= _dig_vec_lower[ascii_code];
           }
           else
-            *pbuf++= c; // normal character, no escaping needed
+            *pbuf++= c; // Normal character, no escaping needed.
         }
-        if (pbuf > buf + (sizeof(buf) - 6))
+        if (pbuf > buf + (sizeof(buf) - 6))     // magic number
         {
-          /* no room in 'buf' for next char, so flush it */
-          rc|= String::append(buf, pbuf - buf);
+          // No room in 'buf' for next char, so flush it.
+          rc|= string_buf.append(buf, pbuf - buf);
           pbuf= buf; // back to buf's start
         }
       }
-      /* flush any chars left in 'buf' */
-      rc|= String::append(buf, pbuf - buf);
+      // Flush any chars left in 'buf'.
+      rc|= string_buf.append(buf, pbuf - buf);
     }
     malloc_error|= rc;
   }
@@ -1027,7 +1037,7 @@ bool Opt_trace_stmt::Buffer::append(cons
       rc= true;
     else
 #endif
-      rc= String::append(str, length);
+      rc= string_buf.append(str, length);
     malloc_error|= rc;
   }
   return rc;
@@ -1044,8 +1054,8 @@ bool Opt_trace_stmt::Buffer::append(char
   }
   else
   {
-    // no need for escaping chr, given how this function is used */
-    rc= String::append(chr);
+    // No need for escaping chr, given how this function is used.
+    rc= string_buf.append(chr);
     malloc_error|= rc;
   }
   return rc;
@@ -1056,11 +1066,13 @@ bool Opt_trace_stmt::Buffer::append(char
 
 Opt_trace_disable_I_S::Opt_trace_disable_I_S(Opt_trace_context *ctx_arg,
                                              bool disable_arg) :
+  ctx(ctx_arg),
+  stmt(ctx ? ctx->current_stmt_in_gen : NULL),
   disable(disable_arg)
 {
   if (likely(!disable))
     return;
-  ctx= ctx_arg;
+
   if (ctx != NULL)
   {
     /* Disable in to-be-created future traces */
@@ -1069,7 +1081,6 @@ Opt_trace_disable_I_S::Opt_trace_disable
     saved_ctx_cannot_change_settings= ctx->cannot_change_settings;
     /* Mark that this disabling is mandatory for all: */
     ctx->cannot_change_settings= true;
-    stmt= ctx->current_stmt_in_gen;
     if (stmt != NULL)
     {
       /* Disable also in the current trace */
@@ -1080,7 +1091,7 @@ Opt_trace_disable_I_S::Opt_trace_disable
 }
 
 
-Opt_trace_disable_I_S::~Opt_trace_disable_I_S(void)
+Opt_trace_disable_I_S::~Opt_trace_disable_I_S()
 {
   if (likely(!disable))
     return;

=== modified file 'sql/opt_trace.h'
--- a/sql/opt_trace.h	2010-12-19 14:24:03 +0000
+++ b/sql/opt_trace.h	2011-01-11 08:15:46 +0000
@@ -18,12 +18,14 @@
 
 #include "my_config.h"  // OPTIMIZER_TRACE
 #include "sql_array.h"  // Dynamic_array
-class THD;
 #include "my_base.h"    // ha_rows
 #include "sql_string.h" // String
 #include "sql_list.h"   // because sql_cmd.h needs it
 #include "sql_cmd.h"    // for enum_sql_command
 
+struct st_schema_table;
+struct TABLE_LIST;
+
 /**
    @file
    API for the Optimizer trace (WL#5257)
@@ -56,17 +58,17 @@ class THD;
   dictionary or Perl's associative array or hash or STL's hash_map.
   @li "arrays" (ordered set of values); equivalent to Python's and Perl's list
   or STL's vector.
-  @li "values": a value can be a string, number, boolean, null, which we all call
-  "scalars", or be an object, array.
+  @li "values": a value can be a string, number, boolean, null,
+  which we all call "scalars", or be an object, array.
 
-  For example (after "<<" are explanations which are not part of output):
+  For example (explanations after "<<" are not part of output):
 @verbatim
   {                           << start of top object
     "first_name": "Gustave",  << key/value pair (value is string)
     "last_name": "Eiffel",    << key/value pair (value is string)
     "born": 1832,             << key/value pair (value is integer)
     "contributions_to": [     << key/value pair (value is array)
-       {                      << 1st item of array is an object (here a building)
+       {                      << 1st item of array is an object (a building)
          "name": "Eiffel tower",
          "location": Paris
        },                     << end of 1st item of array
@@ -190,21 +192,21 @@ class THD;
   Opt_trace_array ota(trace, "semijoin_strategy_choice");
 @endverbatim
 
-  this creates an array for key "semijoin_strategy_choice". We are going to
+  This creates an array for key "semijoin_strategy_choice". We are going to
   list possible semijoin strategy choices.
 
 @verbatim
   Opt_trace_object oto(trace);
 @endverbatim
 
-  this creates an object without key (normal, it's in an array). This
+  This creates an object without key (normal, it's in an array). This
   object will describe one single strategy choice.
 
 @verbatim
   oto.add("strategy", "FirstMatch");
 @endverbatim
 
-  this adds a key/value pair to the just-created object: key is "strategy",
+  This adds a key/value pair to the just-created object: key is "strategy",
   value is "FirstMatch". This is the strategy to be described in the
   just-created object.
 
@@ -214,7 +216,7 @@ class THD;
   oto.add("chosen", (pos->sj_strategy == SJ_OPT_FIRST_MATCH));
 @endverbatim
 
-  this adds 3 key/value pairs: cost of strategy, number of records produced
+  This adds 3 key/value pairs: cost of strategy, number of records produced
   by this strategy, and whether this strategy is chosen.
 
   After that, there is similar code for other semijoin strategies.
@@ -290,8 +292,10 @@ class THD;
   @li Use only simple characters for key names: a-ZA-Z_# (to denote a
   number), and no space.
 
+??? to denote a number ???
+
   @li Keep in mind than in an object, keys are not ordered; an application may
-  parse the JSON output and output it again with keys' order changed; thus
+  parse the JSON output and output it again with keys order changed; thus
   when order matters, use an array (which may imply having anonymous objects
   as items of the array, with keys inside the anonymous objects, see how it's
   done in JOIN::optimize_steps()). Keep in mind that in an object keys should
@@ -321,8 +325,8 @@ class Opt_trace_stmt;
 
   @li When adding a value (scalar or array or object) to an array, or adding a
   key/value pair to an object, we need this outer object or array (from now
-  on, we will call "structure" an "object or array", as both are structured
-  types).
+  on, we will use the term "structure" for "object or array",
+  as both are structured types).
 
   @li The most natural way would be that the outer object would be passed in
   argument to the adder (the function which wants to add the value or
@@ -334,7 +338,8 @@ class Opt_trace_stmt;
   modifying many function prototypes.
   Example (in gdb "backtrace" notation: inner frames first):
 @verbatim
-    #0  Item_in_subselect::single_value_transformer - opens an object for key "transformation"
+    #0  Item_in_subselect::single_value_transformer
+        - opens an object for key "transformation"
     #1  Item_in_subselect::select_in_like_transformer - does no tracing
     #2  Item_allany_subselect::select_transformer - does no tracing
     #3  JOIN::prepare - opens an object for key "join_preparation"
@@ -344,28 +349,32 @@ class Opt_trace_stmt;
 
   @li So, as we cannot practically pass the object down, we rather maintain a
   "current object or array" in the Opt_trace_context context; it's a pointer
-  to an instance of Opt_trace_struct, and the function deep down (frame #0) grabs
-  it from the context, where it was depositted by the function high up (frame
-  #13 in the last example).
+  to an instance of Opt_trace_struct, and the function deep down (frame #0)
+  grabs it from the context, where it was depositted by the function high up
+  (frame #13 in the last example).
 */
 class Opt_trace_context
 {
 public:
-  Opt_trace_context(void);
-  ~Opt_trace_context(void);
+  Opt_trace_context();
+  ~Opt_trace_context();
 
   /**
-     Flags' names for @@@@optimizer_trace variable of @c sys_vars.cc :
+     Names of flags for @@@@optimizer_trace variable of @c sys_vars.cc :
      @li "enabled" = tracing enabled
      @li "end_marker" = see parameter of @ref Opt_trace_context::start
      @li "one_line"= see parameter of @ref Opt_trace_context::start
      @li "default".
   */
   static const char *flag_names[];
+
   /** Flags' numeric values for @@@@optimizer_trace variable */
+  // the style guide says enum enum_flag_value { ... }
+  // the style guide does *not* mandate uppercase enum values .....
   enum flag_value {
     FLAG_DEFAULT= 0, FLAG_ENABLED= 1, FLAG_END_MARKER= 2, FLAG_ONE_LINE= 4
   };
+
   /**
      Features' names for @@@@optimizer_trace_features variable of
      @c sys_vars.cc:
@@ -375,13 +384,16 @@ public:
      @li "default".
   */
   static const char *feature_names[];
+
   /** Features' numeric values for @@@@optimizer_trace_features variable */
+  // These are actually decimal bit values(?)
+  // so maybe write 1 << 0, 1 << 1, and so on???
   enum feature_value {
     GREEDY_SEARCH= 1,
     RANGE_OPTIMIZER= 2, ///@todo join cache, semijoin...
     /*
-      if you add here, update feature_value of empty implementation
-      and FEATURES_DEFAULT!
+      If you add here, update feature_value of empty implementation
+      and default_features!
     */
     /**
        Anything unclassified, including the top object (thus, by "inheritance
@@ -391,15 +403,15 @@ public:
     */
     MISC= 128
   };
-  static const feature_value FEATURES_DEFAULT;
+  static const feature_value default_features;
 
   /**
      Starts a new trace.
-     @param  need_it_for_I_S  should trace produce output suitable for
-                              information_schema, or only send to DBUG
-     @param  end_marker       for a key/(object|array) pair, should the key be
+     @param  need_it_for_I_S  Should trace produce output suitable for
+                              information_schema, or only send to DBUG?
+     @param  end_marker       For a key/(object|array) pair, should the key be
                               repeated in a comment when the object|array
-                              closes, like
+                              closes? like
 @verbatim
                               "key_foo": {
                                            multi-line blah
@@ -409,14 +421,14 @@ public:
                               JSON. Note that YAML supports #-prefixed
                               comments (we would just need to put the next
                               item's "," before the current item's "#").
-     @param  one_line         should the trace be on a single line without
-                              indentation (more compact for network transfer
-                              to programs, less human-readable)
-     @param  offset           offset for restricting trace production
-     @param  limit            limit for restricting trace production
-     @param  max_mem_size     maximum allowed for cumulated size of all
-                              remembered traces
-     @param  features         only those optimizer features should be traced
+     @param  one_line         Should the trace be on a single line without
+                              indentation? (More compact for network transfer
+                              to programs, less human-readable.)
+     @param  offset           Offset for restricting trace production.
+     @param  limit            Limit for restricting trace production.
+     @param  max_mem_size     Maximum allowed for cumulated size of all
+                              remembered traces.
+     @param  features         Only these optimizer features should be traced.
 
      @retval false            ok
      @retval true             error (OOM)
@@ -425,22 +437,25 @@ public:
              long offset, long limit, ulong max_mem_size,
              ulonglong features);
   /** Ends the current trace */
-  void end(void);
+  void end();
+
   /** Returns whether there is a current trace */
-  bool is_started(void) const { return current_stmt_in_gen != NULL; }
+  bool is_started() const { return current_stmt_in_gen != NULL; }
+
   /** Returns the current open Object Or Array */
-  Opt_trace_struct *get_current_struct(void) const;
+  Opt_trace_struct *get_current_struct() const;
+
   /**
      Returns a pointer to the last bytes of the current trace, 0-terminated.
      @param  size  how many last bytes are wanted
   */
   const char *get_tail(size_t size);
+
   /**
      Brainwash: deletes all remembered traces and resets counters regarding
-     OFFSET/LIMIT (so that the next statement is considered as "at offset
-     0").
+     OFFSET/LIMIT (so that the next statement is considered as "at offset 0").
   */
-  void reset(void);
+  void reset();
 
   /**
      Set the "original query" for the current statement.
@@ -462,7 +477,7 @@ private:
      - to output traces "oldest first" in OPTIMIZER_TRACE
      - to preserve traces "newest first" when optimizer-trace-offset<0
      - to delete a trace in the middle of the list when it is permanently out
-     of the offset/limit showable window.
+       of the offset/limit showable window.
   */
   Opt_trace_stmt *oldest_stmt_to_show;
   Opt_trace_stmt *newest_stmt_to_show; ///< Newest remembered statement trace
@@ -473,6 +488,7 @@ private:
      OFFSET/LIMIT
   */
   ha_rows since_offset_0;
+  // ha_rows seems strange, why not int/long?
 
   /**
      Trace which is currently being generated, where structures are being
@@ -506,15 +522,17 @@ private:
      false: they should support only DBUG (or nothing, if non-debug binary)
   */
   bool support_I_S;
-  bool end_marker;  ///< copy of parameter of Opt_trace_context::start
-  bool one_line;    ///< copy of parameter of Opt_trace_context::start
-  long offset;      ///< copy of parameter of Opt_trace_context::start
-  long limit;       ///< copy of parameter of Opt_trace_context::start
-  size_t max_mem_size; ///< copy of parameter of Opt_trace_context::start
+  bool end_marker;        ///< copy of parameter of Opt_trace_context::start
+  bool one_line;          ///< copy of parameter of Opt_trace_context::start
+  long offset;            ///< copy of parameter of Opt_trace_context::start
+  long limit;             ///< copy of parameter of Opt_trace_context::start
+  size_t max_mem_size;    ///< copy of parameter of Opt_trace_context::start
   feature_value features; ///< copy of parameter of Opt_trace_context::start
 
-  /** Whether the settings above may be changed for a new trace */
+  /** Whether the settings above may be changed for a new trace. */
+  // comment is positive, variable name is negative.
   bool cannot_change_settings;
+
   /**
      Find and delete unneeded traces.
      For example if OFFSET=-1,LIMIT=1, only the last trace is needed. When a
@@ -523,22 +541,34 @@ private:
      @param  all  if true, ignore OFFSET and thus delete everything
   */
   void purge(bool purge_all); ///< find and delete unneeded traces
-  /** put trace in list of traces to show in OPTIMIZER_TRACE */
+
+  /** Put trace in list of traces to show in OPTIMIZER_TRACE. */
   void link_to_shown(Opt_trace_stmt *stmt);
-  /** remove trace from list of traces to show in OPTIMIZER_TRACE */
+
+  /** Remove trace from list of traces to show in OPTIMIZER_TRACE. */
   void unlink_from_shown(Opt_trace_stmt *stmt);
-  /** put trace in list of unneeded traces */
+
+  /** Put trace in list of unneeded traces. */
   void link_to_del(Opt_trace_stmt *stmt);
-  /** remove trace from list of unneeded traces */
+
+  /** Remove trace from list of unneeded traces. */
   void unlink_from_del(Opt_trace_stmt *stmt);
+
   /** compute maximum allowed memory size for trace 'for_stmt'*/
   size_t allowed_mem_size(const Opt_trace_stmt *for_stmt) const;
 
+  // Having too many friends is asking for trouble ....
+  // Opt_trace_stmt   needs only a const getter for one_line.
+  // Opt_trace_struct needs a couple of const getters, plus current_stmt_in_gen
+  // Opt_trace_disable_I_S actually changes the state of the context object!
+  // Opt_trace_iterator needs only oldest_stmt_to_show and limit
+  // and finally: the free function does not need friendship.
+
   friend class Opt_trace_stmt;
   friend class Opt_trace_struct;
   friend class Opt_trace_disable_I_S;
   friend class Opt_trace_iterator;
-  friend void opt_trace_print_expanded_query(THD *thd);
+  //friend void opt_trace_print_expanded_query(THD *thd);
 };
 
 
@@ -565,7 +595,8 @@ private: // Except other classes in this
   Opt_trace_stmt(Opt_trace_context *ctx_arg, Opt_trace_stmt *parent,
                  bool support_I_S_arg);
   /** Ends a trace; destruction may not be possible immediately though */
-  void end(void);
+  void end();
+
   bool started;
   Opt_trace_context *ctx;     ///< context
   Opt_trace_stmt *parent;     ///< parent trace
@@ -577,30 +608,30 @@ private: // Except other classes in this
   bool support_I_S;
 
   /**
-     Extension of class String, for storing query or trace.
-
-     We want to prevent users from calling String functions which allocate
-     memory, because we want to record their malloc error status. So we make
-     them accessible only through our wrappers, thanks to private
-     inheritance.
+     A wrapper of class String, for storing query or trace.
   */
-  class Buffer: private String
+  class Buffer
   {
-private:
+  private:
     size_t allowed_mem_size; ///< allowed memory size for this String
     size_t missing_bytes;    ///< how many bytes could not be added
     bool   malloc_error;     ///< whether there was a malloc/realloc() error
-public:
+    String string_buf;
+  public:
     Buffer() : allowed_mem_size(0), missing_bytes(0), malloc_error(false) {}
-    uint32 alloced_length(void) const { return String::alloced_length(); }
-    uint32 length(void) const { return String::length(); }
+    uint32 alloced_length() const { return string_buf.alloced_length(); }
+    uint32 length() const { return string_buf.length(); }
     bool realloc(uint32 arg_length);
-    char *c_ptr_safe(void)
+    char *c_ptr_safe()
     {
       /* Alas, String::c_ptr_safe() does no realloc error checking */
-      return String::c_ptr_safe();
+      return string_buf.c_ptr_safe();
     }
-    inline const char *ptr(void) const { return String::ptr(); }
+    const char *ptr() const { return string_buf.ptr(); }
+
+    CHARSET_INFO *charset() { return string_buf.charset(); }
+    void set_charset(CHARSET_INFO *charset) { string_buf.set_charset(charset); }
+
     /**
        @param  str  String, in this instance's charset
        @param  length  length of string
@@ -622,6 +653,7 @@ public:
     friend class Opt_trace_iterator;
   };
 
+  // Suggest renaming to trace_buffer: much easier to search for it.
   Buffer buffer;         ///< Where the trace is accumulated
   Buffer query_buffer;   ///< Where the query is put
 
@@ -629,19 +661,21 @@ public:
      Nesting level of the current structure.
      The more nested ("deep"), the more indentation spaces we add on the left.
   */
+  // how about current_depth: much easier to search for it.
   int depth;
   /** maximum nesting level so far */
   int max_depth;
   /** whether current structure is empty; one such info per nesting level */
   Dynamic_array<int> level_empty;
+
   /** enter a deeper nesting level */
-  void push(void);
+  void push();
   /** leave current nesting level and go back one level up */
-  void pop(void);
+  void pop();
   /** put comma, newline and indentation */
-  void separator(void);
+  void separator();
   /** put newline and indentation */
-  void next_line(void);
+  void next_line();
 
   /** @sa Opt_trace_context::set_query() */
   bool set_query(const char* query, size_t length, CHARSET_INFO *charset);
@@ -649,11 +683,18 @@ public:
   Opt_trace_stmt *prev, *next; ///< list management
 
   /** By how much we should increase buffer's size when it's becoming full */
+  // At first sight it looks strange that Buffer doesn't handle this itself.
+  // Do we add to the length, or do we multiply (which is more common I think)
   static const size_t buffer_alloc_increment;
   /** Length of an empty trace */
   static const size_t empty_trace_len;
 
 public:
+  const Opt_trace_stmt *prev_() const { return prev; }
+
+  size_t alloced_length() const
+  { return buffer.alloced_length() + query_buffer.alloced_length(); }
+
   /** Turn this on only in unit tests for out-of-memory testing */
   static bool simulate_oom;
 
@@ -661,7 +702,7 @@ public:
   friend class Opt_trace_struct;
   friend class Opt_trace_disable_I_S;
   friend class Opt_trace_iterator;
-  friend void opt_trace_print_expanded_query(THD *thd);
+//  friend void opt_trace_print_expanded_query(THD *thd);
 };
 
 
@@ -671,7 +712,7 @@ public:
 struct Opt_trace_info
 {
   /**
-     String containing trace. It is NUL-terminated, only to aid debugging or
+     String containing trace. It is NULL-terminated, only to aid debugging or
      unit testing; this property is not relied upon in normal server usage.
   */
   const char *trace_ptr;
@@ -708,20 +749,23 @@ public:
    '*' operator.
   */
   Opt_trace_iterator(Opt_trace_context *ctx) :
-    cursor(ctx->oldest_stmt_to_show), row_count(1), limit(ctx->limit) {}
-  void operator++(int); ///< advances iterator to next trace
+    cursor(ctx->oldest_stmt_to_show), row_count(1), limit(ctx->limit)
+  {}
+
+  ///< Advances iterator to next trace.
+  // A bit strange to implement postfix iterator, rather than prefix.
+  void operator++(int);
+
   /**
      Returns information about the trace on which the iterator is
      positioned.
+     @note Returns object by value!!!
   */
   Opt_trace_info operator*();
-  /** Needed to compare the iterator with the list's end */
-  bool operator!=(const Opt_trace_iterator &other) const
-  { return cursor != other.cursor; }
-  static Opt_trace_iterator end;   ///< List's end
+
+  bool at_end() const { return cursor == NULL; }
+
 private:
-  /** Only to construct the 'end' iterator */
-  Opt_trace_iterator(void) : cursor(NULL) {}
   Opt_trace_stmt *cursor; ///< trace which the iterator is positioned on
   ha_rows row_count;      ///< how many traces yielded so far
   ha_rows limit;          ///< yield "empty" after yielding that many traces
@@ -734,13 +778,13 @@ private:
    When you want to add a structure to the trace, you create an instance of
    Opt_trace_object or Opt_trace_array, then you add information to it with
    add(), then the destructor closes the OOA (we use RAII, Resource
+                                         ^^^ ???
    Acquisition Is Initialization).
 */
 class Opt_trace_struct
 {
 protected:
   /**
-     Constructor.
      @param  ctx_arg  Optimizer trace context for this structure
      @param  requires_key_arg  whether this structure requires/forbids keys
                       for values put inside it (an object requires them, an
@@ -761,7 +805,7 @@ protected:
     /* A first inlined test */
     if (unlikely(ctx_arg != NULL) && ctx_arg->is_started())
     {
-      /* tracing enabled: must fully initialize the structure */
+      // Tracing enabled: must fully initialize the structure.
       do_construct(ctx_arg->current_stmt_in_gen,
                    ctx_arg->current_stmt_in_gen->current_struct,
                    requires_key_arg, key, feature);
@@ -771,7 +815,7 @@ protected:
       dummy.
     */
   }
-  ~Opt_trace_struct(void) { if (unlikely(started)) do_destruct(); }
+  ~Opt_trace_struct() { if (unlikely(started)) do_destruct(); }
 
 public:
 
@@ -799,7 +843,7 @@ public:
 
      In the most performance-critical case (release binary with
      tracing compiled in but not enabled at runtime), we don't want function
-     calls, which is why we have an inline if() in each add() method below.
+     calls, which is why we have an inline if() in each add() function below.
      In an optimizer-intensive test (as in BUG#50595 with a 20-table plan
      search, see mysql-test/t/bug50595.test), this inlining took
      @c greedy_search()'s clock time from 2.6 secs down to 2.0 secs.
@@ -807,13 +851,18 @@ public:
      @verbatim <some Opt_trace_struct>.add().add().add() @endverbatim
      which have 3 if()s (one per @c add()), it has been measured that wrapping
      that inside a single if(), like
-     @verbatim if(started) { <some Opt_trace_struct>.add().add().add() } @endverbatim
+     @verbatim if(started) { <some Opt_trace_struct>.add().add().add() }
+     @endverbatim
      degraded performance (from 2.0 to 2.1 secs). Adding if() in one single
-     code line caused de-inlining of add() methods in several unrelated places
+     code line caused de-inlining of add() functions in several unrelated places
      and the performance degraded. So we don't do it. The performance impact of
      multiple chained add() seems comparable to a single add() anyway.
      Removing @c likely() also increases from 2.0 to 2.1, so we leave
      them.
+
+     // I don't need to know all these details about milliseconds here ....
+     // This should be a documentation about the interface.
+
      The same test against a vanilla tree without this WL, took 1.9 secs; so
      does the WL tree with optimizer trace compiled out.
      So the important figure is that this WL takes the common case from 1.9 to
@@ -838,6 +887,7 @@ public:
       return *this;
     return do_add(key, value, strlen(value), false);
   }
+
   /**
      Adds a value (of string type) to the structure. No key is specified, so
      it adds only the value (the structure must thus be an array).
@@ -850,6 +900,7 @@ public:
       return *this;
     return do_add(NULL, value, strlen(value), false);
   }
+
   /**
      Like add_alnum() but supports any UTF8 characters in 'value'.
      Will "escape" 'value' to be JSON-compliant.
@@ -864,6 +915,7 @@ public:
       return *this;
     return do_add(key, value, val_length, true);
   }
+
   /** Variant of add_utf8() for adding to an array (no key) */
   Opt_trace_struct& add_utf8(const char *value, size_t val_length)
   {
@@ -871,6 +923,7 @@ public:
       return *this;
     return do_add(NULL, value, val_length, true);
   }
+
   /** Variant of add_utf8() where 'value' is 0-terminated */
   Opt_trace_struct& add_utf8(const char *key,
                              const char *value)
@@ -879,6 +932,7 @@ public:
       return *this;
     return do_add(key, value, strlen(value), true);
   }
+
   /** Variant of add_utf8() where 'value' is 0-terminated */
   Opt_trace_struct& add_utf8(const char *value)
   {
@@ -886,6 +940,7 @@ public:
       return *this;
     return do_add(NULL, value, strlen(value), true);
   }
+
   /**
      Add a value (of Item type) to the structure. The Item should be a
      condition (like a WHERE clause) which will be pretty-printed into the
@@ -990,7 +1045,7 @@ public:
     structure before it goes out of scope. Don't use it unless RAII mandates
     a new scope which mandates re-indenting lots of code lines.
   */
-  void end(void) { if (unlikely(started)) do_destruct(); }
+  void end() { if (unlikely(started)) do_destruct(); }
 
 private:
   /** Full initialization. @sa Opt_trace_struct::Opt_trace_struct */
@@ -1000,7 +1055,7 @@ private:
                     const char *key,
                     Opt_trace_context::feature_value feature);
   /** Really does destruction */
-  void do_destruct(void);
+  void do_destruct();
   /**
      Really adds to the object. @sa add().
      @param  escape  do JSON-compliant escaping of 'value'.
@@ -1022,6 +1077,7 @@ private:
                  NULL otherwise.
   */
   void add_struct(const char *key);
+
   /**
      Emits a JSON syntax error.
      @param key  key involved in the error, NULL if there is no key.
@@ -1029,6 +1085,7 @@ private:
      When adding a value (or array or object) to an array, or a key/value pair
      to an object, we need this outer array or object.
 
+Sentence below is very hard to read, too many ()()()
      It would be possible, when trying to add a key to an array (which is wrong
      in JSON) (or similarly when trying to add a value without any key to an
      object), to catch it at compilation time, if the outer object was passed an
@@ -1041,13 +1098,13 @@ private:
      Opt_trace_struct), and the adder grabs it from the context.
 
      As this current structure is of type "object or array", we cannot do
-     compile-time checks that only suitable methods are used. A call to @c
+     compile-time checks that only suitable functions are used. A call to @c
      add(key,value) is necessarily legal for the compiler as the structure may
      be an object, though it will be wrong in case the structure is actually
      an array at run-time. Thus we have the risk of an untested particular
      situation where the current structure is not an object (but an array)
      though the code expected it to be one. We catch that at run-time:
-     Opt_trace_struct methods detect wrong usage, like adding a value to an
+     Opt_trace_struct functions detect wrong usage, like adding a value to an
      object without specifying a key, and then they:
      @li in debug build, assert
      @li in release builds, emit a warning string in the trace and should not
@@ -1063,22 +1120,34 @@ private:
   Opt_trace_struct& operator=(const Opt_trace_struct&);
 
   bool started; ///< Whether the structure does tracing
+
   /**
      Whether the structure requires/forbids keys for values inside it.
      1: this is an object. 0: this is an array. Other: incorrect.
 
+     it is unclear at this point why it is int8 rather than bool
+
      @note The canonical way would be to not have such int8 per instance, but
      rather have a pure virtual method Opt_trace_struct::requires_key(),
+C++ has member functions, not methods.
      overloaded by Opt_trace_object (returning 1) and by Opt_trace_array
      (returning 0). But this would have drawbacks:
      @li it would add a vtbl pointer to each instance which takes even more
      space than int8
+-- this depends on alignment (you happen to have a bool as a neighbour..)
      @li it would add requires_key() function calls which cost more than
      reading one int8
+-- you don't know that, until measured on different platforms.
+-- reading an int8 may require masking operations, unless
+-- your platform is byte addressable
      @li Opt_trace_object::requires_key() would not be accessible from
      Opt_trace_struct::construct() (which would complicate coding), whereas the
      int8 is.
+-- virtual function not accessible to CTOR/DTOR: true
+-- OK, so I buy this argument, but not the others :-)
   */
+  // Hand-coded typechecking is generally a bad idea,
+  // but since you need the information in the base CTOR/DTOR: OK.
   int8 requires_key;
   Opt_trace_stmt *stmt;  ///< Trace owning the structure
   /** Parent structure ("outer structure" of the structure) */
@@ -1129,8 +1198,9 @@ public:
   */
   Opt_trace_object(Opt_trace_context *ctx, const char *key,
                    Opt_trace_context::feature_value feature=
-                   Opt_trace_context::MISC) :
-  Opt_trace_struct(ctx, true, key, feature) {}
+                   Opt_trace_context::MISC)
+    : Opt_trace_struct(ctx, true, key, feature)
+  {}
   /**
      Constructs an object. No key is specified, so the object is just a value
      (serves for the single root object, or for adding the object to an array).
@@ -1138,8 +1208,9 @@ public:
   */
   Opt_trace_object(Opt_trace_context *ctx,
                    Opt_trace_context::feature_value feature=
-                   Opt_trace_context::MISC) :
-  Opt_trace_struct(ctx, true, NULL, feature) {}
+                   Opt_trace_context::MISC)
+    : Opt_trace_struct(ctx, true, NULL, feature)
+  {}
 
 };
 
@@ -1159,8 +1230,9 @@ public:
   */
   Opt_trace_array(Opt_trace_context *ctx, const char *key,
                   Opt_trace_context::feature_value feature=
-                  Opt_trace_context::MISC) :
-  Opt_trace_struct(ctx, false, key, feature) {}
+                  Opt_trace_context::MISC)
+    : Opt_trace_struct(ctx, false, key, feature)
+  {}
   /**
      Constructs an array. No key is specified, so the array is just a value
      (serves for adding the object to an array).
@@ -1168,16 +1240,17 @@ public:
   */
   Opt_trace_array(Opt_trace_context *ctx,
                   Opt_trace_context::feature_value feature=
-                  Opt_trace_context::MISC) :
-  Opt_trace_struct(ctx, false, NULL, feature) {}
+                  Opt_trace_context::MISC)
+    : Opt_trace_struct(ctx, false, NULL, feature)
+  {}
 };
 
 
 /**
    Instantiate an instance of this class for specific, punctual cases where
    optimizer trace should write only to DBUG and not I_S. Example:
-   @c QUICK_RANGE_SELECT::dbug_dump() writes to the I_S trace and DBUG a
-   "range_select" object. This is good when called from
+   @c QUICK_RANGE_SELECT::dbug_dump() writes a "range_select" object 
+   to the I_S trace and DBUG. This is good when called from
    @c SQL_SELECT::test_quick_select(). But it is also called by @c TEST_join()
    (from @c JOIN::optimize()), and there the produced I_S trace is
    undesirable, so it is silenced with such object below (trace only goes to
@@ -1191,12 +1264,12 @@ class Opt_trace_disable_I_S
 public:
   /**
      Constructor
-     @param  ctx_arg  context
-     @param  disable_arg  whether the instance should really disable
+     @param  ctx_arg      Context.
+     @param  disable_arg  Whether the instance should really disable
                           anything. If false, the object is dummy. If true,
                           tracing-to-I_S is disabled at construction and
                           re-enabled at destruction.
-     @details a dummy instance is there only for RAII reasons. Imagine we want
+     @details A dummy instance is there only for RAII reasons. Imagine we want
      to do this:
 @verbatim
      {
@@ -1224,6 +1297,7 @@ public:
   Opt_trace_disable_I_S(Opt_trace_context *ctx_arg, bool disable_arg);
   /** Destructor. Re-enables tracing. */
   ~Opt_trace_disable_I_S();
+
 private:
   Opt_trace_context *ctx; ///< context to disable/re-enable
   Opt_trace_stmt *stmt;   ///< trace to disable/re-enable
@@ -1236,13 +1310,18 @@ private:
 };
 
 
+// I believe the macros below are gone in newer versions of the code.
+// Anyways: the names aren't very good, and the formatting was bad ...
+
 /**
    Grabs the current structure from the context and call its methods.
    Most of the time, there is a direct reference to this structure available
    (because it has been instantianted a few lines earlier), then prefer using
    it than this macro.
 */
-#define OPT_TRACE(trace, action) do {                                  \
+#define OPT_TRACE(trace, action) \
+  do \
+  {                                                                    \
     if (unlikely((trace) != NULL))                                     \
       if (unlikely((trace)->is_started()))                             \
         (trace)->get_current_struct()->action;                         \
@@ -1252,7 +1331,9 @@ private:
 /**
    Executes an action only if tracing is started.
 */
-#define OPT_TRACE2(trace, action) do {                                  \
+#define OPT_TRACE2(trace, action) \
+  do \
+  {                                                                     \
     if (unlikely((trace) != NULL))                                      \
       if (unlikely((trace)->is_started()))                              \
         action;                                                         \
@@ -1266,7 +1347,6 @@ private:
 
 //@{
 
-struct TABLE_LIST;
 /**
   Start tracing a THD's actions (generally at a statement's start).
   @param  thd  the THD
@@ -1278,6 +1358,7 @@ struct TABLE_LIST;
   sub-statement in the call chain), and this function decides to trace
   (respectively not trace) the sub-statement, it returns "true"
   (resp. false). Each sub-statement is responsible for ending the trace which it
+()()()()()
   has started.
 */
 bool opt_trace_start(THD *thd, const TABLE_LIST *tbl,
@@ -1328,7 +1409,6 @@ static inline bool opt_trace_set_query(O
 */
 int fill_optimizer_trace_info(THD *thd, TABLE_LIST *tables, Item *cond);
 
-struct st_schema_table;
 /**
    Create fields' descriptions of information_schema.OPTIMIZER_TRACE
    @retval 0 ok
@@ -1361,10 +1441,12 @@ class Opt_trace_object
 public:
   Opt_trace_object(Opt_trace_context *ctx, const char *key,
                    Opt_trace_context::feature_value feature=
-                   Opt_trace_context::MISC) {}
+                   Opt_trace_context::MISC)
+  {}
   Opt_trace_object(Opt_trace_context *ctx,
                    Opt_trace_context::feature_value feature=
-                   Opt_trace_context::MISC) {}
+                   Opt_trace_context::MISC)
+  {}
   Opt_trace_object& add_alnum(const char *key, const char *value)
   { return *this; }
   Opt_trace_object& add_utf8(const char *key,
@@ -1379,7 +1461,7 @@ public:
   Opt_trace_object& add(const char *key, longlong value) { return *this; }
   Opt_trace_object& add(const char *key, ulonglong value) { return *this; }
   Opt_trace_object& add(const char *key, double value) { return *this; }
-  void end(void) {}
+  void end() {}
 };
 
 /** Empty implementation used when optimizer trace is not compiled in */
@@ -1388,10 +1470,12 @@ class Opt_trace_array
 public:
   Opt_trace_array(Opt_trace_context *ctx, const char *key,
                   Opt_trace_context::feature_value feature=
-                  Opt_trace_context::MISC) {}
+                  Opt_trace_context::MISC)
+  {}
   Opt_trace_array(Opt_trace_context *ctx,
                   Opt_trace_context::feature_value feature=
-                  Opt_trace_context::MISC) {}
+                  Opt_trace_context::MISC)
+  {}
   Opt_trace_array& add_alnum(const char *value) { return *this; }
   Opt_trace_array& add_utf8(const char *value, size_t val_length)
   { return *this; }
@@ -1404,7 +1488,7 @@ public:
   Opt_trace_array& add(longlong value) { return *this; }
   Opt_trace_array& add(ulonglong value) { return *this; }
   Opt_trace_array& add(double value) { return *this; }
-  void end(void) {}
+  void end() {}
 };
 
 /** Empty implementation used when optimizer trace is not compiled in */
@@ -1452,12 +1536,14 @@ public:
 /**
    Helper to put the database/table name in the trace
    @param  t  TABLE* pointer
+why macro, why not an ordinary function?
 */
 #define add_utf8_table(t)                                               \
   add_utf8("database", (t)->s->db.str, (t)->s->db.length).              \
   add_utf8("table", (t)->alias)
 
-#if !defined(DBUG_OFF) && !defined(OPTIMIZER_TRACE)
+#if !defined(DBUG_OFF) && !defined(OPTIMIZER_TRACE) \
+ && !defined(OPTIMIZER_TRACE_UNITTEST)
 
 /*
   A debug binary without optimizer trace compiled in, will miss some

=== modified file 'sql/opt_trace2server.cc'
--- a/sql/opt_trace2server.cc	2010-12-19 14:24:03 +0000
+++ b/sql/opt_trace2server.cc	2011-01-11 08:15:46 +0000
@@ -19,14 +19,10 @@
    Helpers connecting the optimizer trace to THD or Information Schema. They
    are dedicated "to the server" (hence the file's name).
    In order to create a unit test of the optimizer trace without defining
-   Item_field (and all its parent classes), st_select_lex..., those helpers
-   are defined out of opt_trace.cc.
+   Item_field (and all its parent classes), st_select_lex..., these helpers
+   are defined in opt_trace.cc.
 */
 
-#ifdef USE_PRAGMA_IMPLEMENTATION
-#pragma implementation				// gcc: Class implementation
-#endif
-
 #include "opt_trace.h"
 #include "sql_show.h"  // schema_table_stored_record()
 #include "sql_parse.h" // sql_command_flags
@@ -60,8 +56,8 @@ static bool list_has_optimizer_trace_tab
    Whether a SQL command qualifies for optimizer tracing.
    @param  sql_command  the command
 */
-static inline bool sql_command_can_be_traced
-(enum enum_sql_command sql_command)
+static inline bool
+sql_command_can_be_traced(enum enum_sql_command sql_command)
 {
   /*
     Tracing is limited to a few SQL commands only.
@@ -71,7 +67,7 @@ static inline bool sql_command_can_be_tr
     - they probably don't have anything interesting optimizer-related
     - select_lex for them might be uninitialized and unprintable.
     - SHOW WARNINGS would create an uninteresting trace and thus overwrite the
-    previous interesting one.
+      previous interesting one.
 
     About prepared statements: note that we don't turn on tracing for
     SQLCOM_PREPARE (respectively SQLCOM_EXECUTE), because we don't know yet what
@@ -96,15 +92,17 @@ bool opt_trace_start(THD *thd, const TAB
     We need an optimizer trace:
     * if the user asked for it or
     * if we are using --debug (because the trace serves as a relay for it, for
-    optimizer debug printouts).
-    Additionally, we should not be tracing if:
+      optimizer debug printouts).
+    Additionally, we should *not* be tracing if:
     * command is not interesting (optimizer-wise)
     * query involves I_S.OPTIMIZER_TRACE (we do not want to overwrite the
-    trace while reading it with SELECT).
+      trace while reading it with SELECT).
   */
-  ulonglong var= thd->variables.optimizer_trace;
+  const ulonglong var= thd->variables.optimizer_trace;
   bool need_it_for_I_S= (var & Opt_trace_context::FLAG_ENABLED);
-  bool need_it_for_dbug= false, allocated_here= false;
+  bool need_it_for_dbug= false;
+  bool allocated_here= false;
+
   /* This will be triggered if --debug or --debug=d:opt_trace is used */
   DBUG_EXECUTE("opt_trace", need_it_for_dbug= true;);
   if (!(need_it_for_I_S || need_it_for_dbug))
@@ -113,16 +111,19 @@ bool opt_trace_start(THD *thd, const TAB
     goto disable;
   if (list_has_optimizer_trace_table(tbl))
     goto disable;
+
   /*
     We don't allocate it in THD's MEM_ROOT as it must survive until a next
     statement (SELECT) reads the trace.
   */
   if (thd->opt_trace == NULL)
   {
-    if ((thd->opt_trace= new Opt_trace_context()) == NULL)
+    // (this version of) operator new can never return NULL.
+    if ((thd->opt_trace= new Opt_trace_context) == NULL)
       goto disable;
     allocated_here= true;
   }
+
 start_trace:
   if (thd->opt_trace->start(need_it_for_I_S,
                             (var & Opt_trace_context::FLAG_END_MARKER),
@@ -140,6 +141,7 @@ start_trace:
     goto disable;
   }
   DBUG_RETURN(true); // started all ok
+
 disable:
   /*
     No need to create a trace for this statement.
@@ -156,7 +158,7 @@ disable:
   if (need_it_for_I_S && thd->opt_trace != NULL && thd->opt_trace->is_started())
   {
     need_it_for_I_S= false;
-    goto start_trace;
+    goto start_trace; // Ouch, criss-cross goto!
   }
   DBUG_RETURN(false);
 }
@@ -176,6 +178,7 @@ void opt_trace_print_expanded_query(THD 
   Opt_trace_context * const trace= thd->opt_trace;
   if (trace == NULL || !trace->is_started())
     return;
+
   /*
     In "SELECT stored_func()" , execution of a
     sub-statement of stored_func() may start by opening tables (that's the case
@@ -188,15 +191,16 @@ void opt_trace_print_expanded_query(THD 
     thd->lex->sql_command is SQLCOM_END in this case. So we do a check of the
     command:
   */
+// Too many parens (()) in comment above, hard to read.
   if (!sql_command_can_be_traced(thd->lex->sql_command))
     return;
+
   char buff[1024];
   String str(buff,(uint32) sizeof(buff), system_charset_info);
   str.length(0);
   /**
-     If this statement is not SELECT, what is shown here can be
-     inexact. INSERT SELECT is shown as SELECT. DELETE WHERE is shown
-     as SELECT WHERE.
+     If this statement is not SELECT, what is shown here can be inexact.
+     INSERT SELECT is shown as SELECT. DELETE WHERE is shown as SELECT WHERE.
      This is acceptable given the audience (developers) and the goal (the
      inexact parts are irrelevant for the optimizer).
   */
@@ -211,7 +215,7 @@ void opt_trace_add_select_number(Opt_tra
 {
   if (unlikely(select_number >= INT_MAX))
   {
-    /* clearer than any huge number */
+    // Clearer than any huge number.
     s->add_alnum("select#", "fake");
   }
   else
@@ -230,9 +234,7 @@ int fill_optimizer_trace_info(THD *thd, 
       The list must not change during the iterator's life time. This is ok as
       the life time is only the present block which cannot change the list.
     */
-    for (Opt_trace_iterator it(thd->opt_trace) ;
-         it != Opt_trace_iterator::end ;
-         it++)
+    for (Opt_trace_iterator it(thd->opt_trace) ; !it.at_end() ; it++)
     {
       const Opt_trace_info info= *it;
       restore_record(table, s->default_values);
@@ -242,8 +244,10 @@ int fill_optimizer_trace_info(THD *thd, 
         When literals with introducers are used, see "LiteralsWithIntroducers"
         in this file.
       */
-      table->field[0]->store(info.query_ptr, info.query_length, info.query_charset);
-      table->field[1]->store(info.trace_ptr, info.trace_length, system_charset_info);
+      table->field[0]->store(info.query_ptr, info.query_length,
+                             info.query_charset);
+      table->field[1]->store(info.trace_ptr, info.trace_length,
+                             system_charset_info);
       table->field[2]->store(info.missing_bytes, true);
       table->field[3]->store(info.malloc_error, true);
       if (schema_table_store_record(thd, table))
@@ -265,6 +269,7 @@ ST_FIELD_INFO optimizer_trace_info[]=
   /* name, length, type, value, maybe_null, old_name, open_method */
   {"QUERY", 65535, MYSQL_TYPE_STRING, 0, false, "", SKIP_OPEN_TABLE},
   {"TRACE", 65535, MYSQL_TYPE_STRING, 0, false, 0, SKIP_OPEN_TABLE},
+  // why is this zero, others are ""            ^
   {"MISSING_BYTES_BEYOND_MAX_MEM_SIZE", 20, MYSQL_TYPE_LONG,
    0, false, "", SKIP_OPEN_TABLE},
   {"OS_MALLOC_ERROR", 1, MYSQL_TYPE_TINY, 0, false, "", SKIP_OPEN_TABLE},
@@ -274,13 +279,11 @@ ST_FIELD_INFO optimizer_trace_info[]=
 
 int make_optimizer_trace_table_for_show(THD *thd, ST_SCHEMA_TABLE *schema_table)
 {
-  ST_FIELD_INFO *field_info;
   Name_resolution_context *context= &thd->lex->select_lex.context;
-  int i;
 
-  for (i= 0; schema_table->fields_info[i].field_name != NULL; i++)
+  for (int i= 0; schema_table->fields_info[i].field_name != NULL; i++)
   {
-    field_info= &schema_table->fields_info[i];
+    ST_FIELD_INFO *field_info= &schema_table->fields_info[i];
     Item_field *field= new Item_field(context,
                                       NullS, NullS, field_info->field_name);
     if (field)

=== modified file 'sql/sys_vars.cc'
--- a/sql/sys_vars.cc	2010-11-23 10:21:53 +0000
+++ b/sql/sys_vars.cc	2011-01-11 08:15:46 +0000
@@ -1437,7 +1437,7 @@ static Sys_var_flagset Sys_optimizer_tra
        " and val is one of {on, off, default}",
        SESSION_VAR(optimizer_trace_features), CMD_LINE(REQUIRED_ARG),
        Opt_trace_context::feature_names,
-       DEFAULT(Opt_trace_context::FEATURES_DEFAULT));
+       DEFAULT(Opt_trace_context::default_features));
 
 /** Delete all old optimizer traces */
 static bool optimizer_trace_update(sys_var *self, THD *thd,

=== modified file 'unittest/gunit/CMakeLists.txt'
--- a/unittest/gunit/CMakeLists.txt	2010-09-18 16:25:43 +0000
+++ b/unittest/gunit/CMakeLists.txt	2011-01-11 08:15:46 +0000
@@ -207,7 +207,7 @@ IF (CMAKE_CXX_COMPILER_ID STREQUAL "SunP
 ENDIF()
 
 # Add tests (link them with sql library) 
-SET(TESTS sql_list mdl mdl_mytap my_regex thread_utils opt_trace)
+SET(TESTS sql_list mdl mdl_mytap my_regex thread_utils opt_trace opt_notrace)
 FOREACH(test ${TESTS})
   ADD_EXECUTABLE(${test}-t ${test}-t.cc)
   TARGET_LINK_LIBRARIES(${test}-t gunit sqlgunitlib strings dbug regex)

=== added file 'unittest/gunit/opt_notrace-t.cc'
--- a/unittest/gunit/opt_notrace-t.cc	1970-01-01 00:00:00 +0000
+++ b/unittest/gunit/opt_notrace-t.cc	2011-01-11 08:15:46 +0000
@@ -0,0 +1,21 @@
+#include "my_config.h"
+#include <gtest/gtest.h>
+
+#if defined(OPTIMIZER_TRACE)
+#undef OPTIMIZER_TRACE
+#endif
+#define OPTIMIZER_TRACE_UNITTEST
+
+#include <opt_trace.h>
+
+namespace {
+
+TEST(Foo, Bar)
+{
+  // Fill in more here, to verify that implementations are in sync!!!
+  Opt_trace_context trace;
+  Opt_trace_object oto(&trace);
+  Opt_trace_array  ota(&trace);
+}
+
+}

=== modified file 'unittest/gunit/opt_trace-t.cc'
--- a/unittest/gunit/opt_trace-t.cc	2010-11-24 18:54:26 +0000
+++ b/unittest/gunit/opt_trace-t.cc	2011-01-11 08:15:46 +0000
@@ -28,8 +28,11 @@
 /* Some minimal working implementations to have a working trace... */
 CHARSET_INFO *system_charset_info= &my_charset_utf8_general_ci;
 /* ... end here. */
+// I didn't quite understand the comments above ...
 
-const ulonglong all_features= Opt_trace_context::FEATURES_DEFAULT;
+namespace {
+
+const ulonglong all_features= Opt_trace_context::default_features;
 
 
 /**
@@ -43,11 +46,12 @@ const ulonglong all_features= Opt_trace_
 */
 void check_json_compliance(const char *str, size_t length)
 {
-#if 0
+  return;
   /*
     Read from stdin, eliminate comments, parse as JSON. If invalid, an exception
     is thrown by Python, uncaught, which produces a non-zero error code.
   */
+#ifndef __WIN__
   const char python_cmd[]=
     "python -c \""
     "import json, re, sys;"
@@ -56,22 +60,34 @@ void check_json_compliance(const char *s
     "json.loads(s, 'utf-8')\"";
   // Send the trace to this new process' stdin:
   FILE *fd= popen(python_cmd, "w");
-  ASSERT_NE((void*)NULL, fd);
-  ASSERT_EQ((size_t)1, fwrite(str, length, 1, fd));
+// ASSERT makes sense here: if we fail to run python we must abort.
+// Most of the other uses of ASSERT should be changed to EXPECT,
+// so that we continue to run the rest of the test even if there
+// is a failed assumption.
+  ASSERT_TRUE(NULL != fd);
+  ASSERT_EQ(1U, fwrite(str, length, 1, fd));
   int rc= pclose(fd);
   rc= WEXITSTATUS(rc);
   ASSERT_EQ(0, rc);
 #endif
 }
 
+class TraceContentTest : public ::testing::Test
+{
+public:
+  Opt_trace_context trace;
+};
+
+
+TEST_F(TraceContentTest, ConstructAndDestruct)
+{
+}
+
 
 /** Test empty trace */
-TEST(Trace_content_test, empty)
+TEST_F(TraceContentTest, empty)
 {
-  /* Create a trace */
-  Opt_trace_context trace;
-  ASSERT_EQ(false, trace.start(true, false, false, -1, 1, ULONG_MAX,
-                               all_features));
+  ASSERT_FALSE(trace.start(true, false, false, -1, 1, ULONG_MAX, all_features));
   /*
     Add at least an object to it. A really empty trace ("") is not
     JSON-compliant, at least Python's JSON module raises an exception.
@@ -83,26 +99,24 @@ TEST(Trace_content_test, empty)
   trace.end();
   /* And verify trace's content */
   Opt_trace_iterator it(&trace);
-  ASSERT_EQ(true, it != Opt_trace_iterator::end);
+  EXPECT_FALSE(it.at_end());
   const Opt_trace_info info= *it;
   const char expected[]= "{\n}";
-  ASSERT_STREQ(expected, info.trace_ptr);
-  ASSERT_EQ(sizeof(expected) - 1, info.trace_length);
+  EXPECT_STREQ(expected, info.trace_ptr);
+  EXPECT_EQ(sizeof(expected) - 1, info.trace_length);
   check_json_compliance(info.trace_ptr, info.trace_length);
-  ASSERT_EQ((size_t)0, info.missing_bytes);
-  ASSERT_EQ(false, info.malloc_error);
+  EXPECT_EQ(0U, info.missing_bytes);
+  EXPECT_FALSE(info.malloc_error);
   /* Should be no more traces */
   it++;
-  ASSERT_EQ(false, it != Opt_trace_iterator::end);
+  EXPECT_TRUE(it.at_end());
 }
 
 
 /** Test normal usage */
-TEST(Trace_content_test, normal_usage)
+TEST_F(TraceContentTest, NormalUsage)
 {
-  Opt_trace_context trace;
-  ASSERT_EQ(false, trace.start(true, true, false, -1, 1, ULONG_MAX,
-                               all_features));
+  ASSERT_FALSE(trace.start(true, true, false, -1, 1, ULONG_MAX, all_features));
   {
     Opt_trace_object oto(&trace);
     {
@@ -125,7 +139,7 @@ TEST(Trace_content_test, normal_usage)
   }
   trace.end();
   Opt_trace_iterator it(&trace);
-  ASSERT_EQ(true, it != Opt_trace_iterator::end);
+  EXPECT_FALSE(it.at_end());
   const Opt_trace_info info= *it;
   const char expected[]=
     "{\n"
@@ -152,19 +166,17 @@ TEST(Trace_content_test, normal_usage)
   ASSERT_EQ((size_t)0, info.missing_bytes);
   ASSERT_EQ(false, info.malloc_error);
   it++;
-  ASSERT_EQ(false, it != Opt_trace_iterator::end);
+  EXPECT_TRUE(it.at_end());
 }
 
 
 /**
-   Test Opt_trace_context::get_tail(). Same as Trace_content_test.normal_usage
+   Test Opt_trace_context::get_tail(). Same as TraceContentTest.NormalUsage
    but with a get_tail() in the middle.
 */
-TEST(Trace_content_test, tail)
+TEST_F(TraceContentTest, tail)
 {
-  Opt_trace_context trace;
-  ASSERT_EQ(false, trace.start(true, true, false, -1, 1, ULONG_MAX,
-                               all_features));
+  ASSERT_FALSE(trace.start(true, true, false, -1, 1, ULONG_MAX, all_features));
   {
     Opt_trace_object oto(&trace);
     {
@@ -187,7 +199,7 @@ TEST(Trace_content_test, tail)
   const char *tail= trace.get_tail(40);
   trace.end();
   Opt_trace_iterator it(&trace);
-  ASSERT_EQ(true, it != Opt_trace_iterator::end);
+  EXPECT_FALSE(it.at_end());
   const Opt_trace_info info= *it;
   const char expected[]=
     "{\n"
@@ -216,16 +228,14 @@ TEST(Trace_content_test, tail)
   ASSERT_EQ(false, info.malloc_error);
   ASSERT_EQ(0, strcmp(expected + sizeof(expected) - 1 - 40, tail));
   it++;
-  ASSERT_EQ(false, it != Opt_trace_iterator::end);
+  EXPECT_TRUE(it.at_end());
 }
 
 
 /** Test reaction to malformed JSON (object with value without key) */
-TEST(Trace_content_test, buggy_object)
+TEST_F(TraceContentTest, BuggyObject)
 {
-  Opt_trace_context trace;
-  ASSERT_EQ(false, trace.start(true, true, false, -1, 1, ULONG_MAX,
-                               all_features));
+  ASSERT_FALSE(trace.start(true, true, false, -1, 1, ULONG_MAX, all_features));
   {
     Opt_trace_object oto(&trace);
     {
@@ -248,7 +258,7 @@ TEST(Trace_content_test, buggy_object)
   }
   trace.end();
   Opt_trace_iterator it(&trace);
-  ASSERT_EQ(true, it != Opt_trace_iterator::end);
+  EXPECT_FALSE(it.at_end());
   const Opt_trace_info info= *it;
   const char expected[]=
     "{\n"
@@ -273,14 +283,13 @@ TEST(Trace_content_test, buggy_object)
   ASSERT_EQ((size_t)0, info.missing_bytes);
   ASSERT_EQ(false, info.malloc_error);
   it++;
-  ASSERT_EQ(false, it != Opt_trace_iterator::end);
+  EXPECT_TRUE(it.at_end());
 }
 
 
 /** Test reaction to malformed JSON (array with value with key) */
-TEST(Trace_content_test, buggy_array)
+TEST_F(TraceContentTest, BuggyArray)
 {
-  Opt_trace_context trace;
   ASSERT_EQ(false, trace.start(true, true, false, -1, 1, ULONG_MAX,
                                all_features));
   {
@@ -299,7 +308,7 @@ TEST(Trace_content_test, buggy_array)
   }
   trace.end();
   Opt_trace_iterator it(&trace);
-  ASSERT_EQ(true, it != Opt_trace_iterator::end);
+  EXPECT_FALSE(it.at_end());
   const Opt_trace_info info= *it;
   const char expected[]=
     "{\n"
@@ -319,16 +328,14 @@ TEST(Trace_content_test, buggy_array)
   ASSERT_EQ((size_t)0, info.missing_bytes);
   ASSERT_EQ(false, info.malloc_error);
   it++;
-  ASSERT_EQ(false, it != Opt_trace_iterator::end);
+  EXPECT_TRUE(it.at_end());
 }
 
 
 /** Test Opt_trace_disable_I_S */
-TEST(Trace_content_test, disable_I_S)
+TEST_F(TraceContentTest, DisableIS)
 {
-  Opt_trace_context trace;
-  ASSERT_EQ(false, trace.start(true, true, false, -1, 1, ULONG_MAX,
-                               all_features));
+  ASSERT_FALSE(trace.start(true, true, false, -1, 1, ULONG_MAX, all_features));
   {
     Opt_trace_object oto(&trace);
     {
@@ -360,7 +367,7 @@ TEST(Trace_content_test, disable_I_S)
   }
   trace.end();
   Opt_trace_iterator it(&trace);
-  ASSERT_EQ(true, it != Opt_trace_iterator::end);
+  EXPECT_FALSE(it.at_end());
   const Opt_trace_info info= *it;
   const char expected[]=
     "{\n"
@@ -387,15 +394,16 @@ TEST(Trace_content_test, disable_I_S)
   ASSERT_EQ((size_t)0, info.missing_bytes);
   ASSERT_EQ(false, info.malloc_error);
   it++;
-  ASSERT_EQ(false, it != Opt_trace_iterator::end);
+  EXPECT_TRUE(it.at_end());
 }
 
 /** Helper for Trace_settings_test.offset */
+// Our style guide says "do not transfer by reference".
 static void make_one_trace(Opt_trace_context &trace, const char *name,
                            long offset, long limit)
 {
-  ASSERT_EQ(false, trace.start(true, true, false, offset, limit, ULONG_MAX,
-                               all_features));
+  ASSERT_FALSE(trace.start(true, true, false, offset, limit, ULONG_MAX,
+                           all_features));
   {
     Opt_trace_object oto(&trace);
     oto.add(name, 0LL);
@@ -407,38 +415,40 @@ static void make_one_trace(Opt_trace_con
 /**
    Helper for Trace_settings_test.offset
 
-   @param  names  a NULL-terminated array of "names"
+   @param  trace  The trace context.
+   @param  names  A NULL-terminated array of "names".
 
    Checks that the list of traces is as expected.
    This macro checks that the first trace contains names[0], that the second
    trace contains names[1], etc. That the number of traces is the same as
    the number of elements in "names".
 
-   @note it is a macro, for proper reporting of line numbers in case of
-   assertion failure (if it were a function, the line number in the function
-   would be reported, which is not useful).
+   @note It is a macro, for proper reporting of line numbers in case of
+   assertion failure. SCOPED_TRACE will report line number at the
+   macro expansion site.
 */
-#define check(trace, names) do                                    \
-  {                                                               \
-    Opt_trace_iterator it(&trace);                                  \
-    Opt_trace_info info;                                            \
-    for (const char **name= names ; *name != NULL ; name++)       \
-    {                                                             \
-      ASSERT_EQ(true, it != Opt_trace_iterator::end);                 \
-      info= *it;                                                    \
-      const size_t name_len= strlen(*name);                         \
-      ASSERT_EQ(name_len + 11, info.trace_length);                   \
-      ASSERT_EQ(0, strncmp(info.trace_ptr + 5, *name, name_len));         \
-      ASSERT_EQ((size_t)0, info.missing_bytes);                     \
-      ASSERT_EQ(false, info.malloc_error);                          \
-      it++;                                                         \
-    }                                                             \
-    ASSERT_EQ(false, it != Opt_trace_iterator::end);               \
-  } while (0)
+#define check(trace, names) { SCOPED_TRACE(""); do_check(&trace, names); }
+
+void do_check(Opt_trace_context *trace, const char**names)
+{
+  Opt_trace_iterator it(trace);
+  for (const char **name= names ; *name != NULL ; name++)
+  {
+    EXPECT_FALSE(it.at_end());
+    Opt_trace_info info= *it;
+    const size_t name_len= strlen(*name);
+    ASSERT_EQ(name_len + 11, info.trace_length);
+    ASSERT_EQ(0, strncmp(info.trace_ptr + 5, *name, name_len));
+    ASSERT_EQ((size_t)0, info.missing_bytes);
+    ASSERT_EQ(false, info.malloc_error);
+    it++;
+  }
+  EXPECT_TRUE(it.at_end());
+}
 
 
 /** Test offset/limit variables */
-TEST(Trace_settings_test, offset)
+TEST(TraceSettingsTest, offset)
 {
   Opt_trace_context trace;
   make_one_trace(trace, "100", -1 /* offset */, 1 /* limit */);
@@ -492,7 +502,7 @@ TEST(Trace_settings_test, offset)
 
 
 /** Test truncation by max_mem_size */
-TEST(Trace_settings_test, max_mem_size)
+TEST(TraceSettingsTest, MaxMemSize)
 {
   Opt_trace_context trace;
   ASSERT_EQ(false, trace.start(true, false, false, -1, 1,
@@ -508,7 +518,7 @@ TEST(Trace_settings_test, max_mem_size)
   }
   trace.end();
   Opt_trace_iterator it(&trace);
-  ASSERT_EQ(true, it != Opt_trace_iterator::end);
+  EXPECT_FALSE(it.at_end());
   const Opt_trace_info info= *it;
   const char expected[]=
     "{\n"
@@ -525,13 +535,13 @@ TEST(Trace_settings_test, max_mem_size)
   ASSERT_EQ(false, info.malloc_error);
   ASSERT_EQ(0, strncmp(expected, info.trace_ptr, sizeof(expected) - 1));
   it++;
-  ASSERT_EQ(false, it != Opt_trace_iterator::end);
+  EXPECT_TRUE(it.at_end());
 }
 
 
 
 /** Test how truncation by max_mem_size affects next traces */
-TEST(Trace_settings_test, max_mem_size2)
+TEST(TraceSettingsTest, MaxMemSize2)
 {
   Opt_trace_context trace;
   ASSERT_EQ(false, trace.start(true, false, false, -2, 2,
@@ -550,7 +560,7 @@ TEST(Trace_settings_test, max_mem_size2)
   }
   trace.end();
   Opt_trace_iterator it(&trace);
-  ASSERT_EQ(true, it != Opt_trace_iterator::end);
+  EXPECT_FALSE(it.at_end());
   Opt_trace_info info= *it;
   ASSERT_EQ((size_t)17, info.trace_length);
   ASSERT_EQ((size_t)16, info.missing_bytes);
@@ -562,7 +572,7 @@ TEST(Trace_settings_test, max_mem_size2)
   ASSERT_EQ((size_t)33, info.missing_bytes);
   ASSERT_EQ(false, info.malloc_error);
   it++;
-  ASSERT_EQ(false, it != Opt_trace_iterator::end);
+  EXPECT_TRUE(it.at_end());
   /*
     3rd trace; the first one should automatically be purged, thus the 3rd
     should have a bit of room.
@@ -574,7 +584,7 @@ TEST(Trace_settings_test, max_mem_size2)
   }
   trace.end();
   Opt_trace_iterator it2(&trace);
-  ASSERT_EQ(true, it2 != Opt_trace_iterator::end);
+  EXPECT_FALSE(it2.at_end());
   info= *it2;
   ASSERT_EQ((size_t)0, info.trace_length);
   ASSERT_EQ((size_t)33, info.missing_bytes);
@@ -589,15 +599,14 @@ TEST(Trace_settings_test, max_mem_size2)
   ASSERT_EQ((size_t)19, info.missing_bytes);
   ASSERT_EQ(false, info.malloc_error);
   it2++;
-  ASSERT_EQ(false, it2 != Opt_trace_iterator::end);
+  EXPECT_TRUE(it2.at_end());
 }
 
 
 /** Test reaction to out-of-memory condition */
 #ifndef DBUG_OFF
-TEST(Trace_content_test, out_of_memory)
+TEST_F(TraceContentTest, OutOfMemory)
 {
-  Opt_trace_context trace;
   ASSERT_EQ(false, trace.start(true, false, false, -1, 1, ULONG_MAX,
                                all_features));
   {
@@ -611,7 +620,7 @@ TEST(Trace_content_test, out_of_memory)
   }
   trace.end();
   Opt_trace_iterator it(&trace);
-  ASSERT_EQ(true, it != Opt_trace_iterator::end);
+  EXPECT_FALSE(it.at_end());
   const Opt_trace_info info= *it;
   const char expected[]=
     "{\n"
@@ -624,15 +633,14 @@ TEST(Trace_content_test, out_of_memory)
   ASSERT_EQ((size_t)0, info.missing_bytes);
   ASSERT_EQ(true, info.malloc_error);   // malloc error reported
   it++;
-  ASSERT_EQ(false, it != Opt_trace_iterator::end);
+  EXPECT_TRUE(it.at_end());
 }
 #endif // !DBUG_OFF
 
 
 /** Test filtering by feature */
-TEST(Trace_content_test, filtering_by_feature)
+TEST_F(TraceContentTest, FilteringByFeature)
 {
-  Opt_trace_context trace;
   ASSERT_EQ(false, trace.start(true, false, false, -1, 1, ULONG_MAX,
                                Opt_trace_context::MISC));
   {
@@ -663,7 +671,7 @@ TEST(Trace_content_test, filtering_by_fe
   }
   trace.end();
   Opt_trace_iterator it(&trace);
-  ASSERT_EQ(true, it != Opt_trace_iterator::end);
+  EXPECT_FALSE(it.at_end());
   const Opt_trace_info info= *it;
   const char expected[]=
     "{\n"
@@ -687,14 +695,13 @@ TEST(Trace_content_test, filtering_by_fe
   ASSERT_EQ((size_t)0, info.missing_bytes);
   ASSERT_EQ(false, info.malloc_error);
   it++;
-  ASSERT_EQ(false, it != Opt_trace_iterator::end);
+  EXPECT_TRUE(it.at_end());
 }
 
 
 /** Test escaping of characters */
-TEST(Trace_content_test, escaping)
+TEST_F(TraceContentTest, escaping)
 {
-  Opt_trace_context trace;
   ASSERT_EQ(false, trace.start(true, true, false, -1, 1, ULONG_MAX,
                                all_features));
   // All ASCII 0-127 chars are valid UTF8 encodings
@@ -702,8 +709,8 @@ TEST(Trace_content_test, escaping)
   for (uint c= 0; c < sizeof(all_chars) - 2 ; c++)
     all_chars[c]= c;
   // Now a character with a two-byte code in utf8: ä
-  all_chars[128]= 0xc3;
-  all_chars[129]= 0xa4;
+  all_chars[128]= 0xc3; // warning on windows: truncation of constant value
+  all_chars[129]= 0xa4; // warning on windows: truncation of constant value
   // all_chars is used both as query...
   trace.set_query(all_chars, sizeof(all_chars), system_charset_info);
   {
@@ -713,7 +720,7 @@ TEST(Trace_content_test, escaping)
   }
   trace.end();
   Opt_trace_iterator it(&trace);
-  ASSERT_EQ(true, it != Opt_trace_iterator::end);
+  EXPECT_FALSE(it.at_end());
   const Opt_trace_info info= *it;
   // we get the trace escaped, JSON-compliant:
   const char expected[]=
@@ -730,14 +737,13 @@ TEST(Trace_content_test, escaping)
   ASSERT_EQ(0, memcmp(all_chars, info.query_ptr, sizeof(all_chars)));
   ASSERT_EQ(system_charset_info, info.query_charset);
   it++;
-  ASSERT_EQ(false, it != Opt_trace_iterator::end);
+  EXPECT_TRUE(it.at_end());
 }
 
 
 /** Test how the system handles non-UTF8 characters, a violation of its API */
-TEST(Trace_content_test, non_utf8)
+TEST_F(TraceContentTest, NonUtf8)
 {
-  Opt_trace_context trace;
   ASSERT_EQ(false, trace.start(true, true, false, -1, 1, ULONG_MAX,
                                all_features));
   /*
@@ -775,7 +781,7 @@ TEST(Trace_content_test, non_utf8)
   }
   trace.end();
   Opt_trace_iterator it(&trace);
-  ASSERT_EQ(true, it != Opt_trace_iterator::end);
+  EXPECT_FALSE(it.at_end());
   const Opt_trace_info info= *it;
   // This is UTF8 and thus JSON-compliant; ABC is present
   const char expected[]=
@@ -791,7 +797,9 @@ TEST(Trace_content_test, non_utf8)
   // we get the query unescaped, verbatim, not 0-terminated:
   ASSERT_EQ(0, memcmp(all_chars, info.query_ptr, sizeof(all_chars)));
   it++;
-  ASSERT_EQ(false, it != Opt_trace_iterator::end);
+  EXPECT_TRUE(it.at_end());
 }
 
+}  // namespace
+
 #endif // OPTIMIZER_TRACE

Attachment: [text/bzr-bundle] bzr/tor.didriksen@oracle.com-20110111081546-uee7eww1v6whym0h.bundle
Thread
bzr commit into mysql-next-mr-bugfixing branch (tor.didriksen:3243) WL#5257Tor Didriksen11 Jan
  • Re: bzr commit into mysql-next-mr-bugfixing branch (tor.didriksen:3243)WL#5257Guilhem Bichot26 Jan
Re: bzr commit into mysql-next-mr-bugfixing branch (tor.didriksen:3243)WL#5257Guilhem Bichot3 Feb
  • Re: bzr commit into mysql-next-mr-bugfixing branch (tor.didriksen:3243)WL#5257Tor Didriksen4 Feb
  • Re: bzr commit into mysql-next-mr-bugfixing branch (tor.didriksen:3243)WL#5257Guilhem Bichot16 Feb