List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:January 26 2011 10:57am
Subject:bzr commit into mysql-next-mr-bugfixing branch (guilhem.bichot:3244)
View as plain text  
#At file:///home/mysql_src/bzrrepos_new/mysql-next-mr-opt-backporting-wl4800-applying-Tor-s-review/ based on revid:tor.didriksen@stripped

 3244 Guilhem Bichot	2011-01-26
      Pulled the entire big commit of Tor's review comments; here doing 
      additional edits and a few reverts.

    modified:
      sql/opt_trace.cc
      sql/opt_trace.h
      sql/opt_trace2server.cc
      unittest/gunit/opt_trace-t.cc
=== modified file 'sql/opt_trace.cc'
--- a/sql/opt_trace.cc	2011-01-11 08:15:46 +0000
+++ b/sql/opt_trace.cc	2011-01-26 10:57:25 +0000
@@ -31,8 +31,6 @@
 
 /* Opt_trace_struct class */
 
-const char Opt_trace_struct::brackets[]= { '[', '{', ']', '}' };
-const char *Opt_trace_struct::types[]= { "array", "object" };
 bool Opt_trace_struct::dbug_assert_on_syntax_error= true;
 
 void Opt_trace_struct::syntax_error(const char *key)
@@ -46,14 +44,26 @@ void Opt_trace_struct::syntax_error(cons
   */
   if (key != NULL)
   {
-    stmt->buffer.append(STRING_WITH_LEN("** invalid JSON"
-                                        " (unexpected key \""));
-    stmt->buffer.append(key);
-    stmt->buffer.append(STRING_WITH_LEN("\") ** "));
+    stmt->trace_buffer.append(STRING_WITH_LEN("** invalid JSON"
+                                              " (unexpected key \""));
+    stmt->trace_buffer.append(key);
+    stmt->trace_buffer.append(STRING_WITH_LEN("\") ** "));
   }
   else
-    stmt->buffer.append(STRING_WITH_LEN("** invalid JSON"
-                                        " (missing key) ** "));
+    stmt->trace_buffer.append(STRING_WITH_LEN("** invalid JSON"
+                                              " (missing key) ** "));
+}
+
+
+/** opening and closing symbols for arrays ([])and objects ({}) */
+static const char brackets[]= { '[', '{', ']', '}' };
+static inline char opening_bracket(bool requires_key)
+{
+  return brackets[requires_key];
+}
+static inline char closing_bracket(bool requires_key)
+{
+  return brackets[requires_key + 2];
 }
 
 
@@ -66,7 +76,7 @@ void Opt_trace_struct::do_construct(Opt_
   saved_key= key;
   requires_key= requires_key_arg;
 
-  DBUG_PRINT("opt_trace", ("%s: starting %s", key, types[requires_key]));
+  DBUG_PRINT("opt_trace", ("%s: starting struct", key));
   stmt= stmt_arg;
   parent= parent_arg;
   DBUG_ASSERT(parent == NULL || stmt == parent->stmt);
@@ -92,48 +102,13 @@ void Opt_trace_struct::do_construct(Opt_
     stmt->support_I_S= false; // disable tracing for this struct and children
   }
   if (stmt->support_I_S)
-  {
-    const size_t alloced=   stmt->buffer.alloced_length();
-    const size_t increment= Opt_trace_stmt::buffer_alloc_increment;
-    if ((alloced - stmt->buffer.length()) < (increment / 3))
-    {
-      /*
-        Support for I_S will produce long strings, and there is little free
-        space left in the allocated buffer, so it looks like
-        realloc is soon unavoidable; so let's get many bytes at a time.
-        Note that if this re-allocation fails, or any String::append(), we
-        will get a weird trace; either truncated if the server stops, or maybe
-        with a hole if there is later memory again for the trace's
-        continuation. That will be visible in the OS_MALLOC_ERROR column.
-      */
-      size_t new_size= alloced + increment;
-      size_t max_size= stmt->buffer.allowed_mem_size;
-      /*
-        Determine a safety margin:
-        (A) String::realloc() adds at most ALIGN_SIZE(1) bytes to requested
-        length, so we need to decrement max_size by this amount, to be sure that
-        we don't allocate more than max_size
-        (B) We need to stay at least one byte under that max_size, or the next
-        append() would trigger up-front truncation, which is potentially wrong
-        for a "pre-emptive allocation" as we do here.
-      */
-      const size_t safety_margin= ALIGN_SIZE(1) /* (A) */ + 1 /* (B) */;
-      if (max_size >= safety_margin)
-      {
-        max_size-= safety_margin;
-        if (new_size > max_size) // Don't pre-allocate more than the limit.
-          new_size= max_size;
-        if (new_size >= alloced) // Never shrink string.
-          stmt->buffer.realloc(new_size);
-      }
-    }
-  }
+    stmt->trace_buffer.prealloc();
   if (likely(parent != NULL))
     parent->add_struct(key);
   stmt->current_struct= this;
   if (!stmt->support_I_S)
     return;
-  stmt->buffer.append(brackets[requires_key]);
+  stmt->trace_buffer.append(opening_bracket(requires_key));
   stmt->push();
 }
 
@@ -160,9 +135,9 @@ void Opt_trace_struct::add_key_name(cons
     strncpy(previous_key, key, sizeof(previous_key) - 1);
     previous_key[sizeof(previous_key) - 1]= 0;
 #endif
-    stmt->buffer.append('"');
-    stmt->buffer.append(key);
-    stmt->buffer.append(STRING_WITH_LEN("\": "));
+    stmt->trace_buffer.append('"');
+    stmt->trace_buffer.append(key);
+    stmt->trace_buffer.append(STRING_WITH_LEN("\": "));
   }
 }
 
@@ -179,7 +154,7 @@ void Opt_trace_struct::add_struct(const 
 
 void Opt_trace_struct::do_destruct()
 {
-  DBUG_PRINT("opt_trace", ("%s: ending %s", saved_key, types[requires_key]));
+  DBUG_PRINT("opt_trace", ("%s: ending struct", saved_key));
   DBUG_ASSERT(started);
   /*
     Note:
@@ -196,12 +171,12 @@ void Opt_trace_struct::do_destruct()
   {
     stmt->pop();
     stmt->next_line();
-    stmt->buffer.append(brackets[requires_key + 2]);
+    stmt->trace_buffer.append(closing_bracket(requires_key));
     if (stmt->ctx->end_marker && saved_key != NULL)
     {
-      stmt->buffer.append(STRING_WITH_LEN(" /* "));
-      stmt->buffer.append(saved_key);
-      stmt->buffer.append(STRING_WITH_LEN(" */"));
+      stmt->trace_buffer.append(STRING_WITH_LEN(" /* "));
+      stmt->trace_buffer.append(saved_key);
+      stmt->trace_buffer.append(STRING_WITH_LEN(" */"));
     }
   }
   stmt->support_I_S= save_stmt_support_I_S;
@@ -209,6 +184,11 @@ void Opt_trace_struct::do_destruct()
 }
 
 
+/**
+   @note add() has an up-front if(), hopefully inlined, so that in the common
+   case - tracing run-time disabled - we have no function call. If tracing is
+   enabled, we call do_add().
+*/
 Opt_trace_struct& Opt_trace_struct::do_add(const char *key, const char *val,
                                            const size_t val_length,
                                            bool escape)
@@ -219,7 +199,7 @@ Opt_trace_struct& Opt_trace_struct::do_a
     return *this;
   stmt->separator();
   add_key_name(key);
-  stmt->buffer.append('"');
+  stmt->trace_buffer.append('"');
   /*
     Objects' keys use "normal" characters (A-Za-z0-9_), no escaping
     needed. Same for numeric/bool values. Only string values may need
@@ -235,7 +215,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. Even worse, a single bad character in the expanded query makes
+      character. So just 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
@@ -246,14 +226,14 @@ Opt_trace_struct& Opt_trace_struct::do_a
     */
     CHARSET_INFO *query_charset= stmt->query_buffer.charset();
     if (my_charset_same(query_charset, system_charset_info))
-      stmt->buffer.append_escaped(val, val_length);
+      stmt->trace_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)
       {
-        stmt->buffer.malloc_error= true;
+        stmt->trace_buffer.malloc_error= true;
         return *this;
       }
       uint errors;
@@ -269,13 +249,13 @@ Opt_trace_struct& Opt_trace_struct::do_a
         query_charset as target character set, so escaping would be
         impossible.
       */
-      stmt->buffer.append_escaped(utf8_str, new_length);
+      stmt->trace_buffer.append_escaped(utf8_str, new_length);
       my_free(utf8_str);
     }
   }
   else
-    stmt->buffer.append(val, val_length);
-  stmt->buffer.append('"');
+    stmt->trace_buffer.append(val, val_length);
+  stmt->trace_buffer.append('"');
   return *this;
 }
 
@@ -293,7 +273,7 @@ Opt_trace_struct& Opt_trace_struct::do_a
   stmt->separator();
   add_key_name(key);
   const LEX_CSTRING *readable= &readables[(int)val];
-  stmt->buffer.append(readable->str, readable->length);
+  stmt->trace_buffer.append(readable->str, readable->length);
   return *this;
 }
 
@@ -308,7 +288,7 @@ Opt_trace_struct& Opt_trace_struct::do_a
     return *this;
   stmt->separator();
   add_key_name(key);
-  stmt->buffer.append(buf);
+  stmt->trace_buffer.append(buf);
   return *this;
 }
 
@@ -323,7 +303,7 @@ Opt_trace_struct& Opt_trace_struct::do_a
     return *this;
   stmt->separator();
   add_key_name(key);
-  stmt->buffer.append(buf);
+  stmt->trace_buffer.append(buf);
   return *this;
 }
 
@@ -338,7 +318,7 @@ Opt_trace_struct& Opt_trace_struct::do_a
     return *this;
   stmt->separator();
   add_key_name(key);
-  stmt->buffer.append(buf);
+  stmt->trace_buffer.append(buf);
   return *this;
 }
 
@@ -350,7 +330,7 @@ Opt_trace_struct& Opt_trace_struct::do_a
     return *this;
   stmt->separator();
   add_key_name(key);
-  stmt->buffer.append(STRING_WITH_LEN("null"));
+  stmt->trace_buffer.append(STRING_WITH_LEN("null"));
   return *this;
 }
 
@@ -597,18 +577,17 @@ bool Opt_trace_context::start(bool suppo
   if (new_stmt_support_I_S && offset >= 0)
   {
     /* If outside the offset/limit window, no need to support I_S */
-    if (since_offset_0 < (ulong)offset)
+    if (since_offset_0 < offset)
     {
-      DBUG_PRINT("opt_trace", ("disabled: since_offset_0(%llu) < offset(%lu)",
-                               (ulonglong)since_offset_0, offset));
+      DBUG_PRINT("opt_trace", ("disabled: since_offset_0(%ld) < offset(%ld)",
+                               since_offset_0, offset));
       new_stmt_support_I_S= false;
     }
-    else if (since_offset_0 >= (ulong)(offset + limit))
+    else if (since_offset_0 >= (offset + limit))
     {
-      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));
+      DBUG_PRINT("opt_trace", ("disabled: since_offset_0(%ld) >="
+                               " offset(%ld) + limit(%ld)",
+                               since_offset_0, offset, limit));
       new_stmt_support_I_S= false;
     }
     since_offset_0++;
@@ -640,9 +619,9 @@ bool Opt_trace_context::start(bool suppo
   */
   purge(false);
   /* This purge may have freed space, compute max allowed size: */
-  stmt->buffer.allowed_mem_size= allowed_mem_size(stmt);
+  stmt->trace_buffer.allowed_mem_size= allowed_mem_size(stmt);
   /* Trace is always in UTF8, it's "all" that JSON accepts */
-  stmt->buffer.set_charset(system_charset_info);
+  stmt->trace_buffer.set_charset(system_charset_info);
   DBUG_ASSERT(system_charset_info == &my_charset_utf8_general_ci);
   DBUG_RETURN(false);
 }
@@ -661,7 +640,7 @@ void Opt_trace_context::end()
         Parent regains control, now it needs to be told that its child has
         used space, and thus parent's allowance has shrunk.
       */
-      parent->buffer.allowed_mem_size= allowed_mem_size(parent);
+      parent->trace_buffer.allowed_mem_size= allowed_mem_size(parent);
     }
   }
   /*
@@ -721,7 +700,7 @@ const char *Opt_trace_context::get_tail(
 {
   if (current_stmt_in_gen == NULL)
     return "";
-  Opt_trace_stmt::Buffer *buffer= &(current_stmt_in_gen->buffer);
+  Opt_trace_stmt::Buffer *buffer= &(current_stmt_in_gen->trace_buffer);
   size_t buffer_len= buffer->length();
   const char *ptr= buffer->c_ptr_safe();
   if (buffer_len > size)
@@ -746,7 +725,7 @@ bool Opt_trace_context::set_query(const 
 
 /* Opt_trace_iterator class */
 
-void Opt_trace_iterator::operator++(int)
+void Opt_trace_iterator::next()
 {
   cursor= cursor->next;
   if (row_count >= limit)
@@ -770,18 +749,18 @@ void Opt_trace_iterator::operator++(int)
    to manipulate in a debugger, or to compare in unit tests with
    EXPECT_STREQ).
    c_ptr_safe() may realloc an empty String from 0 bytes to 8 bytes,
-   when it adds the closing NULL.
+   when it adds the closing NUL.
 */
-Opt_trace_info Opt_trace_iterator::operator*()
+Opt_trace_info Opt_trace_iterator::get_value()
 {
   Opt_trace_info ret;
-  ret.trace_ptr=     cursor->buffer.c_ptr_safe();
-  ret.trace_length=  cursor->buffer.length();
+  ret.trace_ptr=     cursor->trace_buffer.c_ptr_safe();
+  ret.trace_length=  cursor->trace_buffer.length();
   ret.query_ptr=     cursor->query_buffer.ptr();
   ret.query_length=  cursor->query_buffer.length();
   ret.query_charset= cursor->query_buffer.charset();
-  ret.missing_bytes= cursor->buffer.missing_bytes;
-  ret.malloc_error=  cursor->buffer.malloc_error ||
+  ret.missing_bytes= cursor->trace_buffer.missing_bytes;
+  ret.malloc_error=  cursor->trace_buffer.malloc_error ||
     cursor->query_buffer.malloc_error;
   return ret;
 }
@@ -790,15 +769,13 @@ Opt_trace_info Opt_trace_iterator::opera
 /* Opt_trace_stmt class */
 
 const size_t Opt_trace_stmt::empty_trace_len= 4; // magic number
-const size_t Opt_trace_stmt::buffer_alloc_increment= 1024;
-
 
 Opt_trace_stmt::Opt_trace_stmt(Opt_trace_context *ctx_arg,
                                Opt_trace_stmt *parent_arg,
                                bool support_I_S_arg):
   started(true), ctx(ctx_arg), parent(parent_arg),
   current_struct(NULL), support_I_S(support_I_S_arg),
-  depth(0), max_depth(0)
+  current_depth(0), max_depth(0)
 {
   DBUG_ASSERT(parent == NULL || ctx == parent->ctx);
   int dummy= 0;
@@ -809,16 +786,16 @@ Opt_trace_stmt::Opt_trace_stmt(Opt_trace
 
 void Opt_trace_stmt::end()
 {
-  DBUG_ASSERT(depth == 0);
-  depth= 0;
+  DBUG_ASSERT(current_depth == 0);
+  current_depth= 0;
   started= false;
   /* Send the full nice trace to DBUG */
   DBUG_EXECUTE("opt_trace",
-               if (buffer.length() <= empty_trace_len)
+               if (trace_buffer.length() <= empty_trace_len)
                { /*  don't spam DBUG with empty trace */ }
                else
                {
-                 const char *trace= buffer.c_ptr_safe();
+                 const char *trace= trace_buffer.c_ptr_safe();
                  DBUG_LOCK_FILE;
                  fputs("Complete optimizer trace:", DBUG_FILE);
                  fputs(trace, DBUG_FILE);
@@ -842,44 +819,44 @@ void Opt_trace_stmt::next_line()
 {
   if (ctx->one_line)
     return;
-  buffer.append('\n');
+  trace_buffer.append('\n');
 
   // You can get away with a single append here:
-  int ix= sizeof(my_spaces) - 2*depth - 1;
+  int ix= sizeof(my_spaces) - 2*current_depth - 1;
   if (ix < 0)
     ix= 0;
-  buffer.append(&my_spaces[ix]);
+  trace_buffer.append(&my_spaces[ix]);
 }
 
 
 void Opt_trace_stmt::push()
 {
   DBUG_ASSERT(support_I_S);
-  depth++;
-  if (depth > max_depth)
+  current_depth++;
+  if (current_depth > max_depth)
   {
     int dummy= 0;
     level_empty.append(dummy);
-    max_depth= depth;
+    max_depth= current_depth;
   }
-  level_empty.at(depth)= true;
+  level_empty.at(current_depth)= true;
 }
 
 
 void Opt_trace_stmt::pop()
 {
   DBUG_ASSERT(support_I_S);
-  depth--;
+  current_depth--;
 }
 
 
 void Opt_trace_stmt::separator()
 {
   // Put a comma first, if we have already written an object at this level.
-  if (!level_empty.at(depth))
-    buffer.append(',');
+  if (!level_empty.at(current_depth))
+    trace_buffer.append(',');
   next_line();
-  level_empty.at(depth)= false;
+  level_empty.at(current_depth)= false;
 }
 
 
@@ -889,33 +866,70 @@ bool Opt_trace_stmt::set_query(const cha
   // Should be called only once per statement.
   DBUG_ASSERT(query_buffer.ptr() == NULL);
   query_buffer.set_charset(charset);
-  // We are taking a bit of space from 'buffer'.
-  if (buffer.alloced_length() >= buffer.allowed_mem_size)
+  // We are taking a bit of space from 'trace_buffer'.
+  if (trace_buffer.alloced_length() >= trace_buffer.allowed_mem_size)
   {
     // Trace buffer already occupies all space.
-    buffer.missing_bytes+= length;
+    trace_buffer.missing_bytes+= length;
     return true;
   }
-  const size_t available= buffer.allowed_mem_size - buffer.alloced_length();
+  const size_t available= trace_buffer.allowed_mem_size -
+    trace_buffer.alloced_length();
   query_buffer.allowed_mem_size= available;
   /* 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)
-    buffer.allowed_mem_size= 0;
+  if (query_buffer.alloced_length() >= trace_buffer.allowed_mem_size)
+    trace_buffer.allowed_mem_size= 0;
   else
-    buffer.allowed_mem_size-= query_buffer.alloced_length();
+    trace_buffer.allowed_mem_size-= query_buffer.alloced_length();
   return rc;
 }
 
 
 /* Opt_trace_stmt::Buffer class */
 
-bool Opt_trace_stmt::Buffer::realloc(uint32 arg_length)
+bool Opt_trace_stmt::Buffer::prealloc()
 {
-  bool rc= string_buf.realloc(arg_length);
-  malloc_error|= rc;
-  return rc;
+  const size_t alloced=   alloced_length();
+  const size_t increment= 1024;
+  if ((alloced - length()) < (increment / 3))
+  {
+    /*
+      Support for I_S will produce long strings, and there is little free
+      space left in the allocated buffer, so it looks like
+      realloc is soon unavoidable; so let's get many bytes at a time.
+      Note that if this re-allocation fails, or any String::append(), we
+      will get a weird trace; either truncated if the server stops, or maybe
+      with a hole if there is later memory again for the trace's
+      continuation. That will be visible in the OS_MALLOC_ERROR column.
+      */
+    size_t new_size= alloced + increment;
+    size_t max_size= allowed_mem_size;
+    /*
+      Determine a safety margin:
+      (A) String::realloc() adds at most ALIGN_SIZE(1) bytes to requested
+      length, so we need to decrement max_size by this amount, to be sure that
+      we don't allocate more than max_size
+      (B) We need to stay at least one byte under that max_size, or the next
+      append() would trigger up-front truncation, which is potentially wrong
+      for a "pre-emptive allocation" as we do here.
+    */
+    const size_t safety_margin= ALIGN_SIZE(1) /* (A) */ + 1 /* (B) */;
+    if (max_size >= safety_margin)
+    {
+      max_size-= safety_margin;
+      if (new_size > max_size) // Don't pre-allocate more than the limit.
+        new_size= max_size;
+      if (new_size >= alloced) // Never shrink string.
+      {
+        bool rc= string_buf.realloc(new_size);
+        malloc_error|= rc;
+        return rc;
+      }
+    }
+  }
+  return false;
 }
 
 
@@ -1070,7 +1084,7 @@ Opt_trace_disable_I_S::Opt_trace_disable
   stmt(ctx ? ctx->current_stmt_in_gen : NULL),
   disable(disable_arg)
 {
-  if (likely(!disable))
+  if (!disable)
     return;
 
   if (ctx != NULL)
@@ -1093,7 +1107,7 @@ Opt_trace_disable_I_S::Opt_trace_disable
 
 Opt_trace_disable_I_S::~Opt_trace_disable_I_S()
 {
-  if (likely(!disable))
+  if (!disable)
     return;
   if (ctx != NULL)
   {

=== modified file 'sql/opt_trace.h'
--- a/sql/opt_trace.h	2011-01-11 08:15:46 +0000
+++ b/sql/opt_trace.h	2011-01-26 10:57:25 +0000
@@ -289,10 +289,8 @@ struct TABLE_LIST;
   can and should often be used. Having a restricted vocabulary helps
   consistency.
 
-  @li Use only simple characters for key names: a-ZA-Z_# (to denote a
-  number), and no space.
-
-??? to denote a number ???
+  @li Use only simple characters for key names: a-ZA-Z_#, and no space. '#'
+  serves to denote a number, like in "select#" .
 
   @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
@@ -369,9 +367,8 @@ public:
   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 {
+  enum {
     FLAG_DEFAULT= 0, FLAG_ENABLED= 1, FLAG_END_MARKER= 2, FLAG_ONE_LINE= 4
   };
 
@@ -386,11 +383,9 @@ public:
   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...
+    GREEDY_SEARCH= (1 << 0),
+    RANGE_OPTIMIZER= (1 << 1), ///@todo join cache, semijoin...
     /*
       If you add here, update feature_value of empty implementation
       and default_features!
@@ -401,7 +396,7 @@ public:
        This feature cannot be disabled by the user; for this it is important
        that it always has biggest flag; flag's value itself does not matter
     */
-    MISC= 128
+    MISC= (1 << 7)
   };
   static const feature_value default_features;
 
@@ -487,8 +482,7 @@ private:
      Number of statements traced so far since "offset 0", for comparison with
      OFFSET/LIMIT
   */
-  ha_rows since_offset_0;
-  // ha_rows seems strange, why not int/long?
+  long since_offset_0;
 
   /**
      Trace which is currently being generated, where structures are being
@@ -524,13 +518,13 @@ private:
   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
   feature_value features; ///< copy of parameter of Opt_trace_context::start
 
-  /** Whether the settings above may be changed for a new trace. */
-  // comment is positive, variable name is negative.
+  /** Whether the settings above may not be changed for a new trace. */
   bool cannot_change_settings;
 
   /**
@@ -568,7 +562,6 @@ private:
   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);
 };
 
 
@@ -621,7 +614,7 @@ private: // Except other classes in this
     Buffer() : allowed_mem_size(0), missing_bytes(0), malloc_error(false) {}
     uint32 alloced_length() const { return string_buf.alloced_length(); }
     uint32 length() const { return string_buf.length(); }
-    bool realloc(uint32 arg_length);
+    bool prealloc(); ///< pro-actively extend buffer if soon short of space
     char *c_ptr_safe()
     {
       /* Alas, String::c_ptr_safe() does no realloc error checking */
@@ -629,7 +622,7 @@ private: // Except other classes in this
     }
     const char *ptr() const { return string_buf.ptr(); }
 
-    CHARSET_INFO *charset() { return string_buf.charset(); }
+    CHARSET_INFO *charset() const { return string_buf.charset(); }
     void set_charset(CHARSET_INFO *charset) { string_buf.set_charset(charset); }
 
     /**
@@ -653,16 +646,14 @@ private: // Except other classes in this
     friend class Opt_trace_iterator;
   };
 
-  // Suggest renaming to trace_buffer: much easier to search for it.
-  Buffer buffer;         ///< Where the trace is accumulated
+  Buffer trace_buffer;   ///< Where the trace is accumulated
   Buffer query_buffer;   ///< Where the query is put
 
   /**
      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;
+  int current_depth;
   /** maximum nesting level so far */
   int max_depth;
   /** whether current structure is empty; one such info per nesting level */
@@ -682,10 +673,6 @@ private: // Except other classes in this
 
   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;
 
@@ -693,7 +680,7 @@ public:
   const Opt_trace_stmt *prev_() const { return prev; }
 
   size_t alloced_length() const
-  { return buffer.alloced_length() + query_buffer.alloced_length(); }
+  { return trace_buffer.alloced_length() + query_buffer.alloced_length(); }
 
   /** Turn this on only in unit tests for out-of-memory testing */
   static bool simulate_oom;
@@ -702,7 +689,6 @@ 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);
 };
 
 
@@ -712,7 +698,7 @@ public:
 struct Opt_trace_info
 {
   /**
-     String containing trace. It is NULL-terminated, only to aid debugging or
+     String containing trace. It is NUL-terminated, only to aid debugging or
      unit testing; this property is not relied upon in normal server usage.
   */
   const char *trace_ptr;
@@ -745,23 +731,19 @@ public:
     positioned either:
     - on the first element (row_count=1 is then correct)
     - or on the end, in which case row_count will not be used, as the user
-    should compare with the 'end' iterator before taking the content with the
-   '*' operator.
+    should test @c at_end() before reading the content with @c get_value().
   */
   Opt_trace_iterator(Opt_trace_context *ctx) :
     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);
+  void next();                             ///< Advances iterator to next trace.
 
   /**
-     Returns information about the trace on which the iterator is
+     Returns information, by value, about the trace on which the iterator is
      positioned.
-     @note Returns object by value!!!
   */
-  Opt_trace_info operator*();
+  Opt_trace_info get_value();
 
   bool at_end() const { return cursor == NULL; }
 
@@ -777,8 +759,7 @@ private:
    Opt_trace_struct is a base class for them.
    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
-                                         ^^^ ???
+   add(), then the destructor closes the structure (we use RAII, Resource
    Acquisition Is Initialization).
 */
 class Opt_trace_struct
@@ -841,35 +822,6 @@ public:
      @return a reference to the structure, useful for chaining like this:
      @verbatim add(x,y).add(z,t).add(u,v) @endverbatim
 
-     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() 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.
-     In calls like
-     @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
-     degraded performance (from 2.0 to 2.1 secs). Adding if() in one single
-     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
-     2.0 secs, but it is hoped that the used testcase (20-table plan) is
-     unusually intensive on the optimizer and thus real-life cases should have
-     a smaller penalty. This will be benchmarked with the QA teams.
-
      String-related add() variants are named add_[something]():
      - to avoid confusing the compiler between:
      add(const char *value, size_t    val_length) and
@@ -1085,12 +1037,12 @@ 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
-     argument of type @c Opt_trace_array* to the adder. Then the @c add(key,val)
-     call would not compile as Opt_trace_array wouldn't feature it.
+     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 adder received, as function
+     parameter, the type of the structure (like @c Opt_trace_array*). Then
+     the @c add(key,val) call would not compile as Opt_trace_array wouldn't
+     feature it.
 
      But as explained in comment of class Opt_trace_context we
      cannot pass down the object, have to maintain a "current object or
@@ -1131,23 +1083,10 @@ Sentence below is very hard to read, too
      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 :-)
+     (returning 0). But Opt_trace_object::requires_key() would not be accessible
+     from Opt_trace_struct::construct() (which would complicate coding), whereas
+     the int8 is.
   */
-  // 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) */
@@ -1170,10 +1109,6 @@ C++ has member functions, not methods.
   */
   bool save_stmt_support_I_S;
 
-  /** opening and closing symbols for arrays ([])and objects ({}) */
-  static const char brackets[];
-  /** human-readable names of structure types */
-  static const char* types[];
 public:
   /**
      Whether a JSON syntax error should cause an assertion in debug binaries;
@@ -1354,11 +1289,10 @@ private:
   @param  sql_command  SQL command being prepared or executed
   @return whether this function decided to trace (and thus the corresponding
   opt_trace_end() should end the trace).
-  @note if tracing was already started (by a top statement above the present
-  sub-statement in the call chain), and this function decides to trace
+  @note if tracing was already started by a top statement above the present
+  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,

=== modified file 'sql/opt_trace2server.cc'
--- a/sql/opt_trace2server.cc	2011-01-11 08:15:46 +0000
+++ b/sql/opt_trace2server.cc	2011-01-26 10:57:25 +0000
@@ -138,7 +138,7 @@ start_trace:
       delete thd->opt_trace;
       thd->opt_trace= NULL;
     }
-    goto disable;
+    DBUG_RETURN(false);
   }
   DBUG_RETURN(true); // started all ok
 
@@ -158,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; // Ouch, criss-cross goto!
+    goto start_trace;
   }
   DBUG_RETURN(false);
 }
@@ -234,9 +234,9 @@ 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.at_end() ; it++)
+    for (Opt_trace_iterator it(thd->opt_trace) ; !it.at_end() ; it.next())
     {
-      const Opt_trace_info info= *it;
+      const Opt_trace_info info= it.get_value();
       restore_record(table, s->default_values);
       /*
         We will put the query, which is in character_set_client, into a column
@@ -267,12 +267,11 @@ int fill_optimizer_trace_info(THD *thd, 
 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 ""            ^
+  {"QUERY", 65535, MYSQL_TYPE_STRING, 0, false, NULL, SKIP_OPEN_TABLE},
+  {"TRACE", 65535, MYSQL_TYPE_STRING, 0, false, NULL, SKIP_OPEN_TABLE},
   {"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},
+   0, false, NULL, SKIP_OPEN_TABLE},
+  {"OS_MALLOC_ERROR", 1, MYSQL_TYPE_TINY, 0, false, NULL, SKIP_OPEN_TABLE},
   {NULL, 0,  MYSQL_TYPE_STRING, 0, true, NULL, 0}
 };
 

=== modified file 'unittest/gunit/opt_trace-t.cc'
--- a/unittest/gunit/opt_trace-t.cc	2011-01-11 08:15:46 +0000
+++ b/unittest/gunit/opt_trace-t.cc	2011-01-26 10:57:25 +0000
@@ -25,15 +25,22 @@
 
 #ifdef OPTIMIZER_TRACE
 
-/* 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 ...
 
 namespace {
 
 const ulonglong all_features= Opt_trace_context::default_features;
 
+/**
+   @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_json_compliance(str, length)              \
+  {                                                     \
+    SCOPED_TRACE("");                                   \
+    do_check_json_compliance(str, length);              \
+  }
 
 /**
    Checks compliance of a trace with JSON syntax rules.
@@ -44,7 +51,7 @@ const ulonglong all_features= Opt_trace_
    @param  str     pointer to trace
    @param  length  trace's length
 */
-void check_json_compliance(const char *str, size_t length)
+void do_check_json_compliance(const char *str, size_t length)
 {
   return;
   /*
@@ -60,15 +67,11 @@ 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 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);
+  EXPECT_EQ(0, rc);
 #endif
 }
 
@@ -99,8 +102,12 @@ TEST_F(TraceContentTest, empty)
   trace.end();
   /* And verify trace's content */
   Opt_trace_iterator it(&trace);
-  EXPECT_FALSE(it.at_end());
-  const Opt_trace_info info= *it;
+  /*
+    ASSERT here, because a failing EXPECT_FALSE would continue into
+    it.get_value() and segfault.
+  */
+  ASSERT_FALSE(it.at_end());
+  const Opt_trace_info info= it.get_value();
   const char expected[]= "{\n}";
   EXPECT_STREQ(expected, info.trace_ptr);
   EXPECT_EQ(sizeof(expected) - 1, info.trace_length);
@@ -108,8 +115,8 @@ TEST_F(TraceContentTest, empty)
   EXPECT_EQ(0U, info.missing_bytes);
   EXPECT_FALSE(info.malloc_error);
   /* Should be no more traces */
-  it++;
-  EXPECT_TRUE(it.at_end());
+  it.next();
+  ASSERT_TRUE(it.at_end());
 }
 
 
@@ -139,8 +146,8 @@ TEST_F(TraceContentTest, NormalUsage)
   }
   trace.end();
   Opt_trace_iterator it(&trace);
-  EXPECT_FALSE(it.at_end());
-  const Opt_trace_info info= *it;
+  ASSERT_FALSE(it.at_end());
+  const Opt_trace_info info= it.get_value();
   const char expected[]=
     "{\n"
     "  \"one array\": [\n"
@@ -160,13 +167,13 @@ TEST_F(TraceContentTest, NormalUsage)
     "    4\n"
     "  ] /* another array */\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);
-  it++;
-  EXPECT_TRUE(it.at_end());
+  EXPECT_EQ((size_t)0, info.missing_bytes);
+  EXPECT_EQ(false, info.malloc_error);
+  it.next();
+  ASSERT_TRUE(it.at_end());
 }
 
 
@@ -199,8 +206,8 @@ TEST_F(TraceContentTest, tail)
   const char *tail= trace.get_tail(40);
   trace.end();
   Opt_trace_iterator it(&trace);
-  EXPECT_FALSE(it.at_end());
-  const Opt_trace_info info= *it;
+  ASSERT_FALSE(it.at_end());
+  const Opt_trace_info info= it.get_value();
   const char expected[]=
     "{\n"
     "  \"one array\": [\n"
@@ -220,15 +227,15 @@ TEST_F(TraceContentTest, tail)
     "    4\n"
     "  ] /* another array */\n"
     "}";
-  ASSERT_STREQ(expected, info.trace_ptr);
-  ASSERT_EQ(sizeof(expected) - 1, info.trace_length);
-  ASSERT_EQ((void *)0, info.query_ptr);
-  ASSERT_EQ((size_t)0, info.query_length);
-  ASSERT_EQ((size_t)0, info.missing_bytes);
-  ASSERT_EQ(false, info.malloc_error);
-  ASSERT_EQ(0, strcmp(expected + sizeof(expected) - 1 - 40, tail));
-  it++;
-  EXPECT_TRUE(it.at_end());
+  EXPECT_STREQ(expected, info.trace_ptr);
+  EXPECT_EQ(sizeof(expected) - 1, info.trace_length);
+  EXPECT_EQ((void *)0, info.query_ptr);
+  EXPECT_EQ((size_t)0, info.query_length);
+  EXPECT_EQ((size_t)0, info.missing_bytes);
+  EXPECT_EQ(false, info.malloc_error);
+  EXPECT_EQ(0, strcmp(expected + sizeof(expected) - 1 - 40, tail));
+  it.next();
+  ASSERT_TRUE(it.at_end());
 }
 
 
@@ -258,8 +265,8 @@ TEST_F(TraceContentTest, BuggyObject)
   }
   trace.end();
   Opt_trace_iterator it(&trace);
-  EXPECT_FALSE(it.at_end());
-  const Opt_trace_info info= *it;
+  ASSERT_FALSE(it.at_end());
+  const Opt_trace_info info= it.get_value();
   const char expected[]=
     "{\n"
     "  \"one array\": [\n"
@@ -278,12 +285,12 @@ TEST_F(TraceContentTest, BuggyObject)
     "    4\n"
     "  ] /* another array */\n"
     "}";
-  ASSERT_STREQ(expected, info.trace_ptr);
-  ASSERT_EQ(sizeof(expected) - 1, info.trace_length);
-  ASSERT_EQ((size_t)0, info.missing_bytes);
-  ASSERT_EQ(false, info.malloc_error);
-  it++;
-  EXPECT_TRUE(it.at_end());
+  EXPECT_STREQ(expected, info.trace_ptr);
+  EXPECT_EQ(sizeof(expected) - 1, info.trace_length);
+  EXPECT_EQ((size_t)0, info.missing_bytes);
+  EXPECT_EQ(false, info.malloc_error);
+  it.next();
+  ASSERT_TRUE(it.at_end());
 }
 
 
@@ -308,8 +315,8 @@ TEST_F(TraceContentTest, BuggyArray)
   }
   trace.end();
   Opt_trace_iterator it(&trace);
-  EXPECT_FALSE(it.at_end());
-  const Opt_trace_info info= *it;
+  ASSERT_FALSE(it.at_end());
+  const Opt_trace_info info= it.get_value();
   const char expected[]=
     "{\n"
     "  \"one array\": [\n"
@@ -323,12 +330,12 @@ TEST_F(TraceContentTest, BuggyArray)
     "    4\n"
     "  ] /* another array */\n"
     "}";
-  ASSERT_STREQ(expected, info.trace_ptr);
-  ASSERT_EQ(sizeof(expected) - 1, info.trace_length);
-  ASSERT_EQ((size_t)0, info.missing_bytes);
-  ASSERT_EQ(false, info.malloc_error);
-  it++;
-  EXPECT_TRUE(it.at_end());
+  EXPECT_STREQ(expected, info.trace_ptr);
+  EXPECT_EQ(sizeof(expected) - 1, info.trace_length);
+  EXPECT_EQ((size_t)0, info.missing_bytes);
+  EXPECT_EQ(false, info.malloc_error);
+  it.next();
+  ASSERT_TRUE(it.at_end());
 }
 
 
@@ -367,8 +374,8 @@ TEST_F(TraceContentTest, DisableIS)
   }
   trace.end();
   Opt_trace_iterator it(&trace);
-  EXPECT_FALSE(it.at_end());
-  const Opt_trace_info info= *it;
+  ASSERT_FALSE(it.at_end());
+  const Opt_trace_info info= it.get_value();
   const char expected[]=
     "{\n"
     "  \"one array\": [\n"
@@ -388,27 +395,26 @@ TEST_F(TraceContentTest, DisableIS)
     "    4\n"
     "  ] /* another array */\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);
-  it++;
-  EXPECT_TRUE(it.at_end());
+  EXPECT_EQ((size_t)0, info.missing_bytes);
+  EXPECT_EQ(false, info.malloc_error);
+  it.next();
+  ASSERT_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,
+static void make_one_trace(Opt_trace_context *trace, const char *name,
                            long offset, long limit)
 {
-  ASSERT_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);
+    Opt_trace_object oto(trace);
     oto.add(name, 0LL);
   }
-  trace.end();
+  trace->end();
 }
 
 
@@ -434,16 +440,16 @@ void do_check(Opt_trace_context *trace, 
   Opt_trace_iterator it(trace);
   for (const char **name= names ; *name != NULL ; name++)
   {
-    EXPECT_FALSE(it.at_end());
-    Opt_trace_info info= *it;
+    ASSERT_FALSE(it.at_end());
+    Opt_trace_info info= it.get_value();
     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_EQ(name_len + 11, info.trace_length);
+    EXPECT_EQ(0, strncmp(info.trace_ptr + 5, *name, name_len));
+    EXPECT_EQ((size_t)0, info.missing_bytes);
+    EXPECT_EQ(false, info.malloc_error);
+    it.next();
   }
-  EXPECT_TRUE(it.at_end());
+  ASSERT_TRUE(it.at_end());
 }
 
 
@@ -451,51 +457,51 @@ void do_check(Opt_trace_context *trace, 
 TEST(TraceSettingsTest, offset)
 {
   Opt_trace_context trace;
-  make_one_trace(trace, "100", -1 /* offset */, 1 /* limit */);
+  make_one_trace(&trace, "100", -1 /* offset */, 1 /* limit */);
   const char *expected_traces0[]= {"100", NULL};
   check(trace, expected_traces0);
-  make_one_trace(trace, "101", -1, 1);
+  make_one_trace(&trace, "101", -1, 1);
   /* 101 should have overwritten 100 */
   const char *expected_traces1[]= {"101", NULL};
   check(trace, expected_traces1);
-  make_one_trace(trace, "102", -1, 1);
+  make_one_trace(&trace, "102", -1, 1);
   const char *expected_traces2[]= {"102", NULL};
   check(trace, expected_traces2);
   check(trace, expected_traces2);
   trace.reset();
   const char *expected_traces_empty[]= {NULL};
   check(trace, expected_traces_empty);
-  make_one_trace(trace, "103", -3, 2);
-  make_one_trace(trace, "104", -3, 2);
-  make_one_trace(trace, "105", -3, 2);
-  make_one_trace(trace, "106", -3, 2);
-  make_one_trace(trace, "107", -3, 2);
-  make_one_trace(trace, "108", -3, 2);
-  make_one_trace(trace, "109", -3, 2);
+  make_one_trace(&trace, "103", -3, 2);
+  make_one_trace(&trace, "104", -3, 2);
+  make_one_trace(&trace, "105", -3, 2);
+  make_one_trace(&trace, "106", -3, 2);
+  make_one_trace(&trace, "107", -3, 2);
+  make_one_trace(&trace, "108", -3, 2);
+  make_one_trace(&trace, "109", -3, 2);
   const char *expected_traces3[]= {"107", "108", NULL};
   check(trace, expected_traces3);
   trace.reset();
   check(trace, expected_traces_empty);
-  make_one_trace(trace, "110", 3, 2);
-  make_one_trace(trace, "111", 3, 2);
-  make_one_trace(trace, "112", 3, 2);
-  make_one_trace(trace, "113", 3, 2);
-  make_one_trace(trace, "114", 3, 2);
-  make_one_trace(trace, "115", 3, 2);
-  make_one_trace(trace, "116", 3, 2);
+  make_one_trace(&trace, "110", 3, 2);
+  make_one_trace(&trace, "111", 3, 2);
+  make_one_trace(&trace, "112", 3, 2);
+  make_one_trace(&trace, "113", 3, 2);
+  make_one_trace(&trace, "114", 3, 2);
+  make_one_trace(&trace, "115", 3, 2);
+  make_one_trace(&trace, "116", 3, 2);
   const char *expected_traces10[]= {"113", "114", NULL};
   check(trace, expected_traces10);
   trace.reset();
   check(trace, expected_traces_empty);
-  make_one_trace(trace, "117", 0, 1);
-  make_one_trace(trace, "118", 0, 1);
-  make_one_trace(trace, "119", 0, 1);
+  make_one_trace(&trace, "117", 0, 1);
+  make_one_trace(&trace, "118", 0, 1);
+  make_one_trace(&trace, "119", 0, 1);
   const char *expected_traces17[]= {"117", NULL};
   check(trace, expected_traces17);
   trace.reset();
-  make_one_trace(trace, "120", 0, 0);
-  make_one_trace(trace, "121", 0, 0);
-  make_one_trace(trace, "122", 0, 0);
+  make_one_trace(&trace, "120", 0, 0);
+  make_one_trace(&trace, "121", 0, 0);
+  make_one_trace(&trace, "122", 0, 0);
   const char *expected_traces20[]= {NULL};
   check(trace, expected_traces20);
 }
@@ -518,8 +524,8 @@ TEST(TraceSettingsTest, MaxMemSize)
   }
   trace.end();
   Opt_trace_iterator it(&trace);
-  EXPECT_FALSE(it.at_end());
-  const Opt_trace_info info= *it;
+  ASSERT_FALSE(it.at_end());
+  const Opt_trace_info info= it.get_value();
   const char expected[]=
     "{\n"
     "  \"one array\": [\n"
@@ -530,12 +536,12 @@ TEST(TraceSettingsTest, MaxMemSize)
     Without truncation the trace would take:
     2+17+3+1+20*100 = 2023
   */
-  ASSERT_EQ((size_t)996, info.trace_length);
-  ASSERT_EQ((size_t)1027, info.missing_bytes); // 996+1027=2023
-  ASSERT_EQ(false, info.malloc_error);
-  ASSERT_EQ(0, strncmp(expected, info.trace_ptr, sizeof(expected) - 1));
-  it++;
-  EXPECT_TRUE(it.at_end());
+  EXPECT_EQ((size_t)996, info.trace_length);
+  EXPECT_EQ((size_t)1027, info.missing_bytes); // 996+1027=2023
+  EXPECT_EQ(false, info.malloc_error);
+  EXPECT_EQ(0, strncmp(expected, info.trace_ptr, sizeof(expected) - 1));
+  it.next();
+  ASSERT_TRUE(it.at_end());
 }
 
 
@@ -560,19 +566,19 @@ TEST(TraceSettingsTest, MaxMemSize2)
   }
   trace.end();
   Opt_trace_iterator it(&trace);
-  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);
-  ASSERT_EQ(false, info.malloc_error);
-  it++;
-  info= *it;
+  ASSERT_FALSE(it.at_end());
+  Opt_trace_info info= it.get_value();
+  EXPECT_EQ((size_t)17, info.trace_length);
+  EXPECT_EQ((size_t)16, info.missing_bytes);
+  EXPECT_EQ(false, info.malloc_error);
+  it.next();
+  info= it.get_value();
   /* 2nd trace completely empty as first trace left no room */
-  ASSERT_EQ((size_t)0, info.trace_length);
-  ASSERT_EQ((size_t)33, info.missing_bytes);
-  ASSERT_EQ(false, info.malloc_error);
-  it++;
-  EXPECT_TRUE(it.at_end());
+  EXPECT_EQ((size_t)0, info.trace_length);
+  EXPECT_EQ((size_t)33, info.missing_bytes);
+  EXPECT_EQ(false, info.malloc_error);
+  it.next();
+  ASSERT_TRUE(it.at_end());
   /*
     3rd trace; the first one should automatically be purged, thus the 3rd
     should have a bit of room.
@@ -584,22 +590,22 @@ TEST(TraceSettingsTest, MaxMemSize2)
   }
   trace.end();
   Opt_trace_iterator it2(&trace);
-  EXPECT_FALSE(it2.at_end());
-  info= *it2;
-  ASSERT_EQ((size_t)0, info.trace_length);
-  ASSERT_EQ((size_t)33, info.missing_bytes);
-  ASSERT_EQ(false, info.malloc_error);
-  it2++;
-  info= *it2;
+  ASSERT_FALSE(it2.at_end());
+  info= it2.get_value();
+  EXPECT_EQ((size_t)0, info.trace_length);
+  EXPECT_EQ((size_t)33, info.missing_bytes);
+  EXPECT_EQ(false, info.malloc_error);
+  it2.next();
+  info= it2.get_value();
   /*
     3rd one had room. A bit less than first, because just reading the second
     with the iterator has reallocated the second from 0 to 8 bytes...
   */
-  ASSERT_EQ((size_t)14, info.trace_length);
-  ASSERT_EQ((size_t)19, info.missing_bytes);
-  ASSERT_EQ(false, info.malloc_error);
-  it2++;
-  EXPECT_TRUE(it2.at_end());
+  EXPECT_EQ((size_t)14, info.trace_length);
+  EXPECT_EQ((size_t)19, info.missing_bytes);
+  EXPECT_EQ(false, info.malloc_error);
+  it2.next();
+  ASSERT_TRUE(it2.at_end());
 }
 
 
@@ -620,20 +626,20 @@ TEST_F(TraceContentTest, OutOfMemory)
   }
   trace.end();
   Opt_trace_iterator it(&trace);
-  EXPECT_FALSE(it.at_end());
-  const Opt_trace_info info= *it;
+  ASSERT_FALSE(it.at_end());
+  const Opt_trace_info info= it.get_value();
   const char expected[]=
     "{\n"
     "  \"one array\": [\n"
     "\n"                    // 200.4 missing
     "  ]\n"
     "}";
-  ASSERT_STREQ(expected, info.trace_ptr);
-  ASSERT_EQ(sizeof(expected) - 1, info.trace_length);
-  ASSERT_EQ((size_t)0, info.missing_bytes);
-  ASSERT_EQ(true, info.malloc_error);   // malloc error reported
-  it++;
-  EXPECT_TRUE(it.at_end());
+  EXPECT_STREQ(expected, info.trace_ptr);
+  EXPECT_EQ(sizeof(expected) - 1, info.trace_length);
+  EXPECT_EQ((size_t)0, info.missing_bytes);
+  EXPECT_EQ(true, info.malloc_error);   // malloc error reported
+  it.next();
+  ASSERT_TRUE(it.at_end());
 }
 #endif // !DBUG_OFF
 
@@ -671,8 +677,8 @@ TEST_F(TraceContentTest, FilteringByFeat
   }
   trace.end();
   Opt_trace_iterator it(&trace);
-  EXPECT_FALSE(it.at_end());
-  const Opt_trace_info info= *it;
+  ASSERT_FALSE(it.at_end());
+  const Opt_trace_info info= it.get_value();
   const char expected[]=
     "{\n"
     "  \"one array\": [\n"
@@ -689,13 +695,13 @@ TEST_F(TraceContentTest, FilteringByFeat
     "    4\n"
     "  ]\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);
-  it++;
-  EXPECT_TRUE(it.at_end());
+  EXPECT_EQ((size_t)0, info.missing_bytes);
+  EXPECT_EQ(false, info.malloc_error);
+  it.next();
+  ASSERT_TRUE(it.at_end());
 }
 
 
@@ -709,8 +715,8 @@ TEST_F(TraceContentTest, 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; // warning on windows: truncation of constant value
-  all_chars[129]= 0xa4; // warning on windows: truncation of constant value
+  all_chars[128]= static_cast<char>(0xc3);
+  all_chars[129]= static_cast<char>(0xa4);
   // all_chars is used both as query...
   trace.set_query(all_chars, sizeof(all_chars), system_charset_info);
   {
@@ -720,24 +726,24 @@ TEST_F(TraceContentTest, escaping)
   }
   trace.end();
   Opt_trace_iterator it(&trace);
-  EXPECT_FALSE(it.at_end());
-  const Opt_trace_info info= *it;
+  ASSERT_FALSE(it.at_end());
+  const Opt_trace_info info= it.get_value();
   // we get the trace escaped, JSON-compliant:
   const char expected[]=
     "{\n"
     "  \"somekey\": \"\\u0000\\u0001\\u0002\\u0003\\u0004\\u0005\\u0006\\u0007\\u0008\\t\\n\\u000b\\u000c\\r\\u000e\\u000f\\u0010\\u0011\\u0012\\u0013\\u0014\\u0015\\u0016\\u0017\\u0018\\u0019\\u001a\\u001b\\u001c\\u001d\\u001e\\u001f !\\\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\\\]^_`abcdefghijklmnopqrstuvwxyz{|}~ä\"\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);
-  ASSERT_EQ(sizeof(all_chars), info.query_length);
+  EXPECT_EQ((size_t)0, info.missing_bytes);
+  EXPECT_EQ(false, info.malloc_error);
+  EXPECT_EQ(sizeof(all_chars), info.query_length);
   // we get the query unescaped, verbatim, not 0-terminated:
-  ASSERT_EQ(0, memcmp(all_chars, info.query_ptr, sizeof(all_chars)));
-  ASSERT_EQ(system_charset_info, info.query_charset);
-  it++;
-  EXPECT_TRUE(it.at_end());
+  EXPECT_EQ(0, memcmp(all_chars, info.query_ptr, sizeof(all_chars)));
+  EXPECT_EQ(system_charset_info, info.query_charset);
+  it.next();
+  ASSERT_TRUE(it.at_end());
 }
 
 
@@ -781,23 +787,23 @@ TEST_F(TraceContentTest, NonUtf8)
   }
   trace.end();
   Opt_trace_iterator it(&trace);
-  EXPECT_FALSE(it.at_end());
-  const Opt_trace_info info= *it;
+  ASSERT_FALSE(it.at_end());
+  const Opt_trace_info info= it.get_value();
   // This is UTF8 and thus JSON-compliant; ABC is present
   const char expected[]=
     "{\n"
     "  \"somekey\": \"????ABC\"\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);
-  ASSERT_EQ(sizeof(all_chars), info.query_length);
+  EXPECT_EQ((size_t)0, info.missing_bytes);
+  EXPECT_EQ(false, info.malloc_error);
+  EXPECT_EQ(sizeof(all_chars), info.query_length);
   // we get the query unescaped, verbatim, not 0-terminated:
-  ASSERT_EQ(0, memcmp(all_chars, info.query_ptr, sizeof(all_chars)));
-  it++;
-  EXPECT_TRUE(it.at_end());
+  EXPECT_EQ(0, memcmp(all_chars, info.query_ptr, sizeof(all_chars)));
+  it.next();
+  ASSERT_TRUE(it.at_end());
 }
 
 }  // namespace

Attachment: [text/bzr-bundle] bzr/guilhem.bichot@oracle.com-20110126105725-16cdxy0mok0fo9d4.bundle
Thread
bzr commit into mysql-next-mr-bugfixing branch (guilhem.bichot:3244) Guilhem Bichot26 Jan