Merge lp:~sinha/drizzle/trunk-bug-621856 into lp:~drizzle-trunk/drizzle/development

[Monty Taylor]

Using regex ,found bunch of functions with unused params in sql_parse.cc file and cursor.h file .
If the changes are fine,then i will start doing the same for rest of the files in drizzled directory.

[Vijay Samuel (vjsamuel)]

Hi Akash,
 Good to see you submitting your first branch. Great job. There are coding standard violations in your branch. Please make it a point to adhere to all the coding guidelines inn the wiki page that I gave you.

I ll just point out some of the obvious one that I could see,
1) assignment operations should be as follows,
   a= b; one space after the = and nonee before.

2) use two space indentations for new blocks of code.

3) braces must be written in new lines.

Please make it a point to fix all the above mentioned and re submit your merge proposal.

Cheers,
 -Vijay

review: Needs Fixing

[Stewart Smith (stewart)]

On Thu, 10 Mar 2011 21:57:07 -0000, Akash Sinha <email address hidden> wrote:
> --- drizzled/cursor.h 2011-03-07 12:31:41 +0000
> +++ drizzled/cursor.h 2011-03-10 21:44:21 +0000
> @@ -319,12 +319,12 @@
> virtual ha_rows estimate_rows_upper_bound()
> { return stats.records+EXTRA_RECORDS; }
>
> - virtual const char *index_type(uint32_t)
> + virtual const char *index_type()
> { assert(0); return "";}

It looks like index_type() is completely unused (apart from being implemented
in every storage engine) - so instead of just removing the parameter
(which shouldn't always be unused anyway) we should remove all the
implementations of it too.

> @@ -349,15 +349,15 @@
> const unsigned char * key,
> key_part_map keypart_map,
> enum ha_rkey_function find_flag);
> - virtual int index_next(unsigned char *) __attribute__ ((warn_unused_result))
> - { return HA_ERR_WRONG_COMMAND; }
> - virtual int index_prev(unsigned char *)
> - { return HA_ERR_WRONG_COMMAND; }
> - virtual int index_first(unsigned char *)
> - { return HA_ERR_WRONG_COMMAND; }
> - virtual int index_last(unsigned char *)
> - { return HA_ERR_WRONG_COMMAND; }
> - virtual int index_next_same(unsigned char *, const unsigned char *, uint32_t);
> + virtual int index_next() __attribute__ ((warn_unused_result))
> + { return HA_ERR_WRONG_COMMAND; }
> + virtual int index_prev()
> + { return HA_ERR_WRONG_COMMAND; }
> + virtual int index_first()
> + { return HA_ERR_WRONG_COMMAND; }
> + virtual int index_last()
> + { return HA_ERR_WRONG_COMMAND; }
> + virtual int index_next_same();

This is incorrect, the buffer being passed in is important and at least
*should* be used by classes implementing them.

> private:
> uint32_t calculate_key_len(uint32_t key_position, key_part_map keypart_map_arg);
> @@ -379,20 +379,20 @@
> bool eq_range, bool sorted);
> virtual int read_range_next();
> int compare_key(key_range *range);
> - virtual int rnd_next(unsigned char *)=0;
> - virtual int rnd_pos(unsigned char *, unsigned char *)=0;
> + virtual int rnd_next()=0;
> + virtual int rnd_pos()=0;

the rnd_pos() method is used and does use the parameters (just not in
the default implementation that does nothing), same with rnd_next() -
that is used.

> virtual int read_first_row(unsigned char *buf, uint32_t primary_key);
> - virtual int rnd_same(unsigned char *, uint32_t)
> + virtual int rnd_same()
> { return HA_ERR_WRONG_COMMAND; }
> - virtual ha_rows records_in_range(uint32_t, key_range *, key_range *)
> + virtual ha_rows records_in_range()
> { return (ha_rows) 10; }

also wrong, these parameters are used by all classes that implement it.

So most of this patch isn't quite right (especially around
Cursor). However, there are some unused bit sthat can work. I'd
recommend starting from scratch on a tree and doing these bits

I'd also be sure to when checking out any virtual function to check
everywhere that calls it as well as everywhere that implements it.

--
Stewart Smith

[Stewart Smith (stewart)]

review: Disapprove

[Akash Sinha (sinha)]

How would i find out exactly from which functions i have to remove
unused params.And moreover i could i say which function is completely
unused.

And also after removing the unused params i ran the "python
tests/dbqp.py" and it didnt return anything wrong.So in case of
virtual functions should i have to go through the entire list of class
functions to check whether that param is being used or not.

Please suggest some better method of doing this.

Akash Sinha

On Fri, Mar 11, 2011 at 10:12 AM, Stewart Smith
<email address hidden> wrote:
> On Thu, 10 Mar 2011 21:57:07 -0000, Akash Sinha <email address hidden> wrote:
>> --- drizzled/cursor.h 2011-03-07 12:31:41 +0000
>> +++ drizzled/cursor.h 2011-03-10 21:44:21 +0000
>> @@ -319,12 +319,12 @@
>>    virtual ha_rows estimate_rows_upper_bound()
>>    { return stats.records+EXTRA_RECORDS; }
>>
>> -  virtual const char *index_type(uint32_t)
>> +  virtual const char *index_type()
>>    { assert(0); return "";}
>
> It looks like index_type() is completely unused (apart from being implemented
> in every storage engine) - so instead of just removing the parameter
> (which shouldn't always be unused anyway) we should remove all the
> implementations of it too.
>
>> @@ -349,15 +349,15 @@
>>                                   const unsigned char * key,
>>                                   key_part_map keypart_map,
>>                                   enum ha_rkey_function find_flag);
>> -  virtual int index_next(unsigned char *) __attribute__ ((warn_unused_result))
>> -   { return  HA_ERR_WRONG_COMMAND; }
>> -  virtual int index_prev(unsigned char *)
>> -   { return  HA_ERR_WRONG_COMMAND; }
>> -  virtual int index_first(unsigned char *)
>> -   { return  HA_ERR_WRONG_COMMAND; }
>> -  virtual int index_last(unsigned char *)
>> -   { return  HA_ERR_WRONG_COMMAND; }
>> -  virtual int index_next_same(unsigned char *, const unsigned char *, uint32_t);
>> +  virtual int index_next() __attribute__ ((warn_unused_result))
>> +   { return  HA_ERR_WRONG_COMMAND; }
>> +  virtual int index_prev()
>> +   { return  HA_ERR_WRONG_COMMAND; }
>> +  virtual int index_first()
>> +   { return  HA_ERR_WRONG_COMMAND; }
>> +  virtual int index_last()
>> +   { return  HA_ERR_WRONG_COMMAND; }
>> +  virtual int index_next_same();
>
> This is incorrect, the buffer being passed in is important and at least
> *should* be used by classes implementing them.
>
>>  private:
>>    uint32_t calculate_key_len(uint32_t key_position, key_part_map keypart_map_arg);
>> @@ -379,20 +379,20 @@
>>                                 bool eq_range, bool sorted);
>>    virtual int read_range_next();
>>    int compare_key(key_range *range);
>> -  virtual int rnd_next(unsigned char *)=0;
>> -  virtual int rnd_pos(unsigned char *, unsigned char *)=0;
>> +  virtual int rnd_next()=0;
>> +  virtual int rnd_pos()=0;
>
> the rnd_pos() method is used and does use the parameters (just not in
> the default implementation that does nothing), same with rnd_next() -
> that is used.
>
>>    virtual int read_first_row(unsigned char *buf, uint32_t primary_key);
>> -  virtual int rnd_same(unsigned char *, uint32_t)
>> +  virtual int rnd_same()
>>      { return HA_ERR_WRONG...

[Akash Sinha (sinha)]

I haven't touched any part of code rather than removing unused params
from the function definiton and testing it.This coding standard
violation the one with assignment thing might be present there in my
code trunk.

--Akash

On Fri, Mar 11, 2011 at 10:01 AM, Vijay Samuel <email address hidden> wrote:
> Review: Needs Fixing
> Hi Akash,
>  Good to see you submitting your first branch. Great job. There are coding standard violations in your branch. Please make it a point to adhere to all the coding guidelines inn the wiki page that I gave you.
>
> I ll just point out some of the obvious one that I could see,
> 1) assignment operations should be as follows,
>   a= b; one space after the = and nonee before.
>
> 2) use two space indentations for new blocks of code.
>
> 3)  braces must be written in new lines.
>
> Please make it a point to fix all the above mentioned and re submit your merge proposal.
>
> Cheers,
>  -Vijay
> --
> https://code.launchpad.net/~sinha/drizzle/trunk-bug-621856/+merge/52931
> You are the owner of lp:~sinha/drizzle/trunk-bug-621856.
>

[Brian Aker (brianaker)]

Thanks!

I'll just verify that the "=0" issue was in the original code. In general if you can fix stuff like this when you pass through the code that is great (and I can see if you are new to the code this might be an issue).

Stewart is right about some of these calls, you should see what warnings the compiler issues when you try to recompile.

[Akash Sinha (sinha)]

While working on this sometimes i get confused what to do.

Suppose there is function " int join_no_more_records(ReadRecord
*info); " in sql_select.h file
Now same function is implemented as " int
join_no_more_records(ReadRecord *) " in sql_select.cc file,Here there
is no params defined in this function in .cc file.

So what should be my action here ? Rremove that ReadRecord * from
sql_select.cc file as well as from sql_select.h file and check if that
function is implemented some other .cc file and rectify that also.

OR should i only check such kind of functions in .cc files as i found
one in sql_parese.cc file i.e function
TableList *Select_Lex::end_nested_join(Session *) as its entry is not
in sql_parse.h file.

Please reply in what manner i should proceed to fix this bug.

Thank you
Akash Sinha

On Fri, Mar 11, 2011 at 10:13 AM, Stewart Smith
<email address hidden> wrote:
> Review: Disapprove
>
> --
> https://code.launchpad.net/~sinha/drizzle/trunk-bug-621856/+merge/52931
> You are the owner of lp:~sinha/drizzle/trunk-bug-621856.
>

Unmerged revisions

2228. By Akash Sinha<email address hidden> on 2011-03-10

Removed functions with unused params

=== modified file 'drizzled/cursor.h'
--- drizzled/cursor.h	2011-03-14 05:40:28 +0000
+++ drizzled/cursor.h	2011-03-15 22:31:52 +0000
@@ -318,12 +318,12 @@
   virtual ha_rows estimate_rows_upper_bound()
   { return stats.records+EXTRA_RECORDS; }
 
-  virtual const char *index_type(uint32_t)
+  virtual const char *index_type()
   { assert(0); return "";}
 
 
-  uint32_t get_index(void) const { return active_index; }
-  virtual int close(void)=0;
+  uint32_t get_index() const { return active_index; }
+  virtual int close()=0;
 
   /**
      @brief
@@ -348,15 +348,15 @@
                                  const unsigned char * key,
                                  key_part_map keypart_map,
                                  enum ha_rkey_function find_flag);
-  virtual int index_next(unsigned char *) __attribute__ ((warn_unused_result))
-   { return  HA_ERR_WRONG_COMMAND; }
-  virtual int index_prev(unsigned char *)
-   { return  HA_ERR_WRONG_COMMAND; }
-  virtual int index_first(unsigned char *)
-   { return  HA_ERR_WRONG_COMMAND; }
-  virtual int index_last(unsigned char *)
-   { return  HA_ERR_WRONG_COMMAND; }
-  virtual int index_next_same(unsigned char *, const unsigned char *, uint32_t);
+  virtual int index_next() __attribute__ ((warn_unused_result))
+   { return  HA_ERR_WRONG_COMMAND; }
+  virtual int index_prev()
+   { return  HA_ERR_WRONG_COMMAND; }
+  virtual int index_first()
+   { return  HA_ERR_WRONG_COMMAND; }
+  virtual int index_last()
+   { return  HA_ERR_WRONG_COMMAND; }
+  virtual int index_next_same();
 
 private:
   uint32_t calculate_key_len(uint32_t key_position, key_part_map keypart_map_arg);
@@ -378,20 +378,20 @@
                                bool eq_range, bool sorted);
   virtual int read_range_next();
   int compare_key(key_range *range);
-  virtual int rnd_next(unsigned char *)=0;
-  virtual int rnd_pos(unsigned char *, unsigned char *)=0;
+  virtual int rnd_next()=0;
+  virtual int rnd_pos()=0;
   virtual int read_first_row(unsigned char *buf, uint32_t primary_key);
-  virtual int rnd_same(unsigned char *, uint32_t)
+  virtual int rnd_same()
     { return HA_ERR_WRONG_COMMAND; }
-  virtual ha_rows records_in_range(uint32_t, key_range *, key_range *)
+  virtual ha_rows records_in_range()
     { return (ha_rows) 10; }
   virtual void position(const unsigned char *record)=0;
-  virtual int info(uint32_t)=0; // see my_base.h for full description
-  virtual uint32_t calculate_key_hash_value(Field **)
+  virtual int info()=0; // see my_base.h for full description
+  virtual uint32_t calculate_key_hash_value()
   { assert(0); return 0; }
   virtual int extra(enum ha_extra_function)
   { return 0; }
-  virtual int extra_opt(enum ha_extra_function operation, uint32_t)
+  virtual int extra_opt(enum ha_extra_function operation)
   { return extra(operation); }
 
   /**
@@ -443,7 +443,7 @@
   /* end of the list of admin commands */
 
   virtual int indexes_are_disabled(void) {return 0;}
-  virtual void append_create_info(String *)
+  virtual void append_create_info()
   {}
   /**
       If index == MAX_KEY then a check for table is made and if index <
@@ -465,7 +465,7 @@
   virtual int get_foreign_key_list(Session *, List<ForeignKeyInfo> *)
   { return 0; }
   virtual uint32_t referenced_by_foreign_key() { return 0;}
-  virtual void free_foreign_key_create_info(char *) {}
+  virtual void free_foreign_key_create_info() {}
 
   /**
     Is not invoked for non-transactional temporary tables.
@@ -521,9 +521,9 @@
     the corresponding 'ha_*' method above.
   */
 
-  virtual int open(const char *, int , uint32_t ) { assert(0); return -1; }
+  virtual int open() { assert(0); return -1; }
   virtual int doOpen(const identifier::Table &identifier, int mode, uint32_t test_if_locked);
-  virtual int doStartIndexScan(uint32_t idx, bool)
+  virtual int doStartIndexScan(uint32_t idx)
   { active_index= idx; return 0; }
   virtual int doEndIndexScan() { active_index= MAX_KEY; return 0; }
   /**
@@ -535,17 +535,17 @@
   */
   virtual int doStartTableScan(bool scan) __attribute__ ((warn_unused_result)) = 0;
   virtual int doEndTableScan() { return 0; }
-  virtual int doInsertRecord(unsigned char *)
-  {
-    return HA_ERR_WRONG_COMMAND;
-  }
-
-  virtual int doUpdateRecord(const unsigned char *, unsigned char *)
-  {
-    return HA_ERR_WRONG_COMMAND;
-  }
-
-  virtual int doDeleteRecord(const unsigned char *)
+  virtual int doInsertRecord()
+  {
+    return HA_ERR_WRONG_COMMAND;
+  }
+
+  virtual int doUpdateRecord()
+  {
+    return HA_ERR_WRONG_COMMAND;
+  }
+
+  virtual int doDeleteRecord()
   {
     return HA_ERR_WRONG_COMMAND;
   }
@@ -578,7 +578,7 @@
     @return  non-0 in case of failure, 0 in case of success.
     When lock_type is F_UNLCK, the return value is ignored.
   */
-  virtual int external_lock(Session *, int)
+  virtual int external_lock()
   {
     return 0;
   }
@@ -590,10 +590,9 @@
   virtual void start_bulk_insert(ha_rows)
   {}
   virtual int end_bulk_insert(void) { return 0; }
-  virtual int index_read(unsigned char *, const unsigned char *,
-                         uint32_t, enum ha_rkey_function)
+  virtual int index_read( enum ha_rkey_function)
    { return  HA_ERR_WRONG_COMMAND; }
-  virtual int index_read_last(unsigned char *, const unsigned char *, uint32_t)
+  virtual int index_read_last()
    { return (errno= HA_ERR_WRONG_COMMAND); }
   /**
     This is called to delete all rows in a table
@@ -609,19 +608,19 @@
     is emulated by doing a 'DELETE FROM t'. HA_ERR_WRONG_COMMAND is
     returned by storage engines that don't support this operation.
   */
-  virtual int reset_auto_increment(uint64_t)
+  virtual int reset_auto_increment()
   { return HA_ERR_WRONG_COMMAND; }
 
-  virtual int analyze(Session *)
+  virtual int analyze()
   { return HA_ADMIN_NOT_IMPLEMENTED; }
 
-  virtual int disable_indexes(uint32_t)
-  { return HA_ERR_WRONG_COMMAND; }
-
-  virtual int enable_indexes(uint32_t)
-  { return HA_ERR_WRONG_COMMAND; }
-
-  virtual int discard_or_import_tablespace(bool)
+  virtual int disable_indexes()
+  { return HA_ERR_WRONG_COMMAND; }
+
+  virtual int enable_indexes()
+  { return HA_ERR_WRONG_COMMAND; }
+
+  virtual int discard_or_import_tablespace()
   { return (errno=HA_ERR_WRONG_COMMAND); }
 
   /* 

=== modified file 'drizzled/sql_parse.cc'
--- drizzled/sql_parse.cc	2011-03-10 13:40:36 +0000
+++ drizzled/sql_parse.cc	2011-03-15 22:31:52 +0000
@@ -1127,7 +1127,7 @@
     - 0, otherwise
 */
 
-TableList *Select_Lex::end_nested_join(Session *)
+TableList *Select_Lex::end_nested_join()
 {
   TableList *ptr;
   NestedJoin *nested_join;
@@ -1536,7 +1536,7 @@
     true  Error
 */
 
-bool update_precheck(Session *session, TableList *)
+bool update_precheck(Session *session)
 {
   const char *msg= 0;
   Select_Lex *select_lex= &session->lex().select_lex;
@@ -1575,7 +1575,7 @@
     true   error
 */
 
-bool insert_precheck(Session *session, TableList *)
+bool insert_precheck(Session *session)
 {
   /*
     Check that we have modify privileges for the first table and