List:Commits« Previous MessageNext Message »
From:Tor Didriksen Date:June 1 2011 12:06pm
Subject:bzr commit into mysql-trunk branch (tor.didriksen:3135) WL#5860
View as plain text  
#At file:///export/home/didrik/repo/trunk-review-jorgen-cost/ based on revid:tor.didriksen@stripped

 3135 Tor Didriksen	2011-06-01
      Review comments for WL#5860: Make COST_VECT a properly ..

    modified:
      sql/handler.cc
      sql/handler.h
      unittest/gunit/cost_estimate-t.cc
=== modified file 'sql/handler.cc'
--- a/sql/handler.cc	2011-05-31 13:13:42 +0000
+++ b/sql/handler.cc	2011-06-01 12:06:38 +0000
@@ -4564,7 +4564,10 @@ handler::multi_range_read_info_const(uin
     else
       cost->add_io(read_time(keyno, n_ranges, total_rows) *
                    Cost_estimate::IO_BLOCK_READ_COST);
-    cost->add_cpu((double)total_rows * ROW_EVALUATE_COST + 0.01);
+    cost->add_cpu(total_rows * ROW_EVALUATE_COST + 0.01);
+    // If either operand is double, the other is converted to double,
+    // and ROW_EVALUATE_COST is a floating-point literal of type double.
+    // So there should be no need for a (C-style) cast.
   }
   return total_rows;
 }

=== modified file 'sql/handler.h'
--- a/sql/handler.h	2011-05-31 13:13:42 +0000
+++ b/sql/handler.h	2011-06-01 12:06:38 +0000
@@ -1052,7 +1052,8 @@ uint16 &mrr_persistent_flag_storage(rang
 char* &mrr_get_ptr_by_idx(range_seq_t seq, uint idx);
 
 /**
-  Used to store optimizer cost estimates
+  Used to store optimizer cost estimates.
+  A note here about 'copy CTOR' and 'assignement operator' ?
 */
 class Cost_estimate
 { 
@@ -1077,26 +1078,26 @@ public:
   {}
 
   /**
-    Returns sum of time-consuming costs, i.e., not counting memory
-    cost
+    Returns sum of time-consuming costs, i.e., not counting memory cost.
   */
-  double total_cost()      { return io_cost + cpu_cost + import_cost; }
-  double get_io_cost()     { return io_cost; }
-  double get_cpu_cost()    { return cpu_cost; }
-  double get_import_cost() { return import_cost; }
-  double get_mem_cost()    { return mem_cost; }
+  double total_cost()      const { return io_cost + cpu_cost + import_cost; }
+  double get_io_cost()     const { return io_cost; }
+  double get_cpu_cost()    const { return cpu_cost; }
+  double get_import_cost() const { return import_cost; }
+  double get_mem_cost()    const { return mem_cost; }
 
   /**
     Reset all costs to zero
   */
-  void zero() { io_cost= cpu_cost= import_cost= mem_cost= 0; }
+  void zzzero() { io_cost= cpu_cost= import_cost= mem_cost= 0; }
+  // This one was unused (except for the unit test).
 
   /**
     Whether or not all costs in the object are zero
     
     @return true if all costs are zero, false otherwise
   */
-  bool is_zero() 
+  bool is_zero() const
   { 
     if (io_cost || cpu_cost || import_cost || mem_cost)
       return false;
@@ -1124,7 +1125,8 @@ public:
     return *this;
   }
 
-  const Cost_estimate operator+ (const Cost_estimate &other)
+  Cost_estimate operator+ (const Cost_estimate &other)
+  // we return object by value here, so the const prefix didn't make much sense.
   {
     Cost_estimate result= *this;
     result+= other;

=== modified file 'unittest/gunit/cost_estimate-t.cc'
--- a/unittest/gunit/cost_estimate-t.cc	2011-05-31 13:13:42 +0000
+++ b/unittest/gunit/cost_estimate-t.cc	2011-06-01 12:06:38 +0000
@@ -18,48 +18,34 @@
 #include "my_config.h"
 #include <gtest/gtest.h>
 
-//#include "item.h"
-// #include "sql_class.h"
-// #include "rpl_handler.h"                        // delegates_init()
 #include "handler.h"
 
 namespace {
 
-class CostEstimateTest : public ::testing::Test
-{
-protected:
-  /*
-    This is the part of the server global things which have to be initialized
-    for this (very simple) unit test. Presumably the list will grow once
-    we start writing tests for more advanced classes.
-    TODO: Move to a common library.
-   */
-  static void SetUpTestCase() {}
-  static void TearDownTestCase() {}
-
-  CostEstimateTest() {}
-
-  virtual void SetUp() {}
-  virtual void TearDown() {}
-};
-
+// Your test fixture is impty, so you don't need it.
+// You want EXPECT_DOUBLE_EQ, see:
+// http://code.google.com/p/googletest/wiki/AdvancedGuide#Floating-Point_Comparison
 
-TEST_F(CostEstimateTest, Basics)
+TEST(CostEstimateTest, Basics)
 {
   Cost_estimate cv1;
 
   EXPECT_EQ(0, cv1.total_cost());
   EXPECT_TRUE(cv1.is_zero());
 
-  cv1.add_io(4.5);
+  // It would be good to give names to all these magic constants.
+  const double initial_io_cost= 4.5;
+
+  cv1.add_io(initial_io_cost);
   EXPECT_FALSE(cv1.is_zero());
-  EXPECT_EQ(4.5, cv1.total_cost());
+  EXPECT_EQ(initial_io_cost, cv1.total_cost());
 
-  cv1.add_cpu(3.3);
+  const double initial_cpu_cost= 3.3;
+  cv1.add_cpu(initial_cpu_cost);
 
-  EXPECT_EQ(3.3, cv1.get_cpu_cost());
-  EXPECT_EQ(4.5, cv1.get_io_cost());
-  EXPECT_EQ(7.8, cv1.total_cost());
+  EXPECT_DOUBLE_EQ(initial_cpu_cost, cv1.get_cpu_cost());
+  EXPECT_DOUBLE_EQ(initial_io_cost, cv1.get_io_cost());
+  EXPECT_DOUBLE_EQ(initial_io_cost + initial_cpu_cost, cv1.total_cost());
   
   EXPECT_EQ(0, cv1.get_mem_cost());
   EXPECT_EQ(0, cv1.get_import_cost());
@@ -74,11 +60,11 @@ TEST_F(CostEstimateTest, Basics)
   EXPECT_EQ(20.3, cv1.total_cost());
 
   EXPECT_FALSE(cv1.is_zero());
-  cv1.zero();
-  EXPECT_TRUE(cv1.is_zero());
+//  cv1.zero();
+//  EXPECT_TRUE(cv1.is_zero());
 }
 
-TEST_F(CostEstimateTest, Operators)
+TEST(CostEstimateTest, Operators)
 {
   Cost_estimate cv_io;
 


Attachment: [text/bzr-bundle] bzr/tor.didriksen@oracle.com-20110601120638-k1shqx22qautq33a.bundle
Thread
bzr commit into mysql-trunk branch (tor.didriksen:3135) WL#5860Tor Didriksen1 Jun