MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Ingo Strüwing Date:October 13 2009 5:59pm
Subject:Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2879)
Bug#40756
View as plain text  
Hi Rafal,

STATUS: Approved pending changes.

REQUIRED: Break of lines, which were shifted into column 81 by fixing
their indentation. See details below.

COMMENTARY: All other changes are fine as far as I can tell. Most
changes are required to make the code to comply with the guidelines.
Some changes may not be strongly required. I annotated such in the
details. Additionally I pointed out some possible oversights. And I
mentioned some personal preferences.

DETAILS:
Rafal Somla, 09.10.2009 14:13:
...

>  2879 Rafal Somla	2009-10-09
>       Bug #40756 - Correct coding guideline violations in backup code.
>       
>       This patch fixes coding guidelines violations which could be found
>       in backup code.
...
> === modified file 'sql/backup/api_types.h'
...
> @@ -72,7 +75,7 @@ class Db_ref
>  
>  public:
>  
> -  /// Construct invalid reference
> +  /// Construct invalid reference.


It is fine to append a dot to a complete sentence. I just wonder if we
have a rule for it? I mean, did I miss it? Did I wrong in the past not
to append a dot everywhere?

...
> @@ -99,10 +102,10 @@ public:
>  protected:
>  
>    // Constructors are made protected as clients of this class are
> -  // not supposed to create instances (see comment inside Table_ref)
> +  // not supposed to create instances (see comment inside Table_ref).


Shouldn't this become a multi-line comment?

...
> @@ -301,13 +306,15 @@ struct Buffer
>    }
>  };
>  
> -// forward declaration
> +
> +// Forward declarations.


For example, this is not a full sentence. At least here, won't the
end-dot be optional? This is not a change request. I'm just curious to
learn your motivation to change it.

...
> === modified file 'sql/backup/backup_engine.h'
...
> @@ -259,286 +258,275 @@ class Backup_driver: public Driver
...
> -   Backup driver can implement its own policy of handling these requests. It can
> -   return immediately from the call and use a separate thread to fill the buffer
...
> +    Backup driver can implement its own policy of handling these requests. It can
> +    return immediately from the call and use a separate thread to fill the buffer
...


By fixing the indentation, some characters shift into column 81. :-(

Same applies to a couple of comments later in this file.

Here is the complete list of lines (I found) that are longer than 80
characters in the files, modified by this patch. Some are modified by
the patch and must be changed, some pre-existed. I recommend to fix the
latter too. (Sorry for the broken lines. Thunderbird...)

==============================================================================
sql/backup/backup_engine.h
==============================================================================
272:    Backup driver can implement its own policy of handling these
requests. It can
273:    return immediately from the call and use a separate thread to
fill the buffer
284:    as described in the documentation of Buffer class. It is
possible to complete
293:                initialized by backup kernel: @c buf.data points to
a memory area
300:                @c size, @c table_num and @c last members of the
buffer structure
308:                completed. This result can be returned only in the
final transfer
311:    @retval PROCESSING The request was accepted but is not completed
yet. Further
351:    @retval READY The driver is ready for synchronization, i.e. it
can accept the
370:    -# Create a validity point of its backup image. The whole backup
image should
489:    completed yet. The buffer will not be used for other purposes
until a further


==============================================================================
sql/backup/backup_info.cc
==============================================================================
296:Backup_info::Ts_hash_node::Ts_hash_node(const String *name)
:name(name), it(NULL)


==============================================================================
sql/backup/backup_test.cc

==============================================================================
46:  protocol->send_result_set_metadata(&field_list,
Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF);


==============================================================================
sql/backup/be_default.cc
==============================================================================
99:  all_tables=
(TABLE_LIST*)my_malloc(tables.count()*sizeof(TABLE_LIST), MYF(MY_WME));
174:  DBUG_RETURN(locking_thd->start_locking_thread("default driver
locking thread"));
294:    error= bitmap_init(&cols, use_bitbuf ? bitbuf : NULL, (n_fields
+ 7) & ~7UL, FALSE);
672:    error= bitmap_init(&cols, use_bitbuf ? bitbuf : NULL, (n_fields
+ 7) & ~7UL, FALSE);
675:    error= unpack_row(NULL, cur_table, n_fields, packed_row, &cols,
&cur_row_end, &length, FALSE);


==============================================================================
sql/backup/kernel.cc
==============================================================================
54:  @section s1 How to use backup kernel API to perform backup and
restore operations
61:  All this is accomplished by creating an instance of
Backup_create_ctx class and
71:                                                 orig_loc); //
prepare for backup
94:                                                   orig_loc); //
prepare for restore
1489:
fatal_error(report_error(ER_BACKUP_CANT_RESTORE_EVENT,ev->describe(buf)));
1877:      backup::LEX_STRING name_lex(snap->engine.name.begin,
snap->engine.name.end);


==============================================================================
sql/backup/restore_info.h
==============================================================================
150:    m_log.report_error(ER_BACKUP_CATALOG_ADD_TABLE, db.name().ptr(),
name.ptr());


==============================================================================
sql/backup/stream_v1.c
==============================================================================
14:  @todo handle errors when creating iterators in functions like
bstream_wr_catalogue()
15:  @todo use data chunk sequence numbers to detect discontinuities in
backup stream.
130:/* Low level i/o operations on backup stream (defined in
stream_v1_carrier.c). */
443:  - @c global_options (2 bytes). Integer global options. Reserved
for future use.
1155:  Each of these chunks contains a list of metadata entries, each
entry containing
1469:  @subsubsection catalog_pos_perdb Catalog coordinates for other
per-database objects
1541:  @param[in] item  pointer to a structure describing object found
is stored here.
1691:  After reading metadata for each object (CREATE statement and/or
extra metadata)
1696:  @retval BSTREAM_EOC   either list was empty or all objects were
read and created


==============================================================================
sql/backup/stream_v1.h
==============================================================================
122:  unsigned long int       table_count;  /**< Number of tables in the
snapshot. */


==============================================================================
sql/backup/stream_v1_transport.c
==============================================================================
1200:  @retval BSTREAM_OK at least one byte was written (or copied to
the output buffer)

...
> === modified file 'sql/backup/backup_info.cc'
...
> @@ -752,15 +793,14 @@ int Backup_info::add_dbs(THD *thd, List<
>  
>    @returns 0 on success, error code otherwise.
>  */
> +
>  int Backup_info::add_all_dbs()
>  {
>    using namespace obs;
>  
>    int res= 0;
>  
> -  /*
> -    Check to see if the user can see all of the databases.
> -  */
> +  // Check to see if the user can see all of the databases.


According to my understanding of MySQL coding guidelines, such change is
definitely optional. While multi-line comments must have /* and */ on
their own lines, single line comments can still have them.

And it is disputed, if an all-line comment may start with //. I do not
reject it, but I raise a question mark. Note that in kernel.cc below I
see the reverse change: All-line C++ style comments changed to C style
comments, which is doubtlessly compliant with the guidelines.

...
> === modified file 'sql/backup/be_default.h'
...
> @@ -20,64 +20,65 @@ const size_t META_SIZE= 1;
...
> -const byte RCD_ONCE=    1U;     // Single data block for record data
> -const byte RCD_FIRST=  (1U<<1); // First data block in buffer for record
> buffer
> -const byte RCD_DATA=   (1U<<2); // Intermediate data block for record buffer
> -const byte RCD_LAST=   (1U<<3); // Last data block in buffer for record
> buffer
> -const byte BLOB_ONCE=   3U;     // Single data block for blob data
> -const byte BLOB_FIRST= (3U<<1); // First data block in buffer for blob buffer
> -const byte BLOB_DATA=  (3U<<2); // Intermediate data block for blob buffer
> -const byte BLOB_LAST=  (3U<<3); // Last data block in buffer for blob buffer
> +const byte RCD_ONCE=    1U;     // Single data block for record data.
> +const byte RCD_FIRST=  (1U<<1); // First data block in buffer for record
> buffer.
> +const byte RCD_DATA=   (1U<<2); // Intermediate data block for record buffer.
> +const byte RCD_LAST=   (1U<<3); // Last data block in buffer for record
> buffer.
> +const byte BLOB_ONCE=   3U;     // Single data block for blob data.
> +const byte BLOB_FIRST= (3U<<1); // First data block in buffer for blob
> buffer.
> +const byte BLOB_DATA=  (3U<<2); // Intermediate data block for blob buffer.
> +const byte BLOB_LAST=  (3U<<3); // Last data block in buffer for blob buffer.


IMHO these change is optional. It is fine to have a dot at the end of
these short descriptions, but it should not be taken as a style
violation if someone does not place a dot at such lines.

...
> === modified file 'sql/backup/be_nodata.h'
...
> @@ -152,9 +153,13 @@ public:
>    { return (ptr= new nodata_backup::Backup(m_tables)) ? OK : ERROR; }
>  
>    result_t get_restore_driver(Restore_driver* &ptr)
> -  { return (ptr= new nodata_backup::Restore(m_tables,::current_thd)) ? OK : ERROR;
> }
> +  {
> +    return (ptr= new nodata_backup::Restore(m_tables,::current_thd)) ?
> +           OK : ERROR;
> +  }
>  
> -  bool is_valid(){ return TRUE; };
> +  bool is_valid()
> +  { return TRUE; };


The change is fine. But IMHO a space before the opening brace would have
been sufficient. And, btw, the patch doesn't change the same thing in
class Default_snapshot.

...
> @@ -932,44 +936,51 @@ int Scheduler::step()
...
> -    DBUG_PRINT("backup_data",("driver counts: total=%u, init=%u, prepare=%u,
> finish=%u.",
> -                              m_count, init_count, prepare_count, finish_count));
> +    DBUG_PRINT("backup_data",("driver counts: "
> +                              "total=%u, init=%u, prepare=%u, finish=%u.",
> +                              m_count, init_count, prepare_count,
> +                              finish_count));


Just for your information. It is not written in the coding guidelines.
So I do not ask for a change. When I started at MySQL I was told that
"we" want to print like: "total: %u  init: %u  prepare: %u  ...".
Perhaps we can adhere to this voluntarily in future work.

...
> === modified file 'sql/backup/kernel.cc'
...
> @@ -176,29 +180,29 @@ static int send_reply(Backup_restore_ctx
...
> -  // Error code insertion for ER_BACKUP_CONTEXT_CREATE.
> +  /* Error code insertion for ER_BACKUP_CONTEXT_CREATE. */


The change is absolutely fine. I just wonder why we do the reverse
changes in this file than in many other files?

...
> === modified file 'sql/backup/stream.h'
...
> @@ -214,15 +224,16 @@ write_summary(const Image_info &info, Ou
>  
>    @retval  ERROR if stream error, OK if no errors.
>  */
> +
>  inline
> -result_t
> -read_header(Image_info &info, Input_stream &s)
> +result_t read_header(Image_info &info, Input_stream &s)


The change is fine. But IMHO the old form was tolerable too.

...
> @@ -231,19 +242,21 @@ read_header(Image_info &info, Input_stre
...
> -  int ret= bstream_rd_catalogue(&s,
> static_cast<st_bstream_image_header*>(&info));
> +  int ret= bstream_rd_catalogue(&s,
> +                               
> static_cast<st_bstream_image_header*>(&info));


The change is in line with the rules. No need to change the change.
However, personally I would prefer in this case:

  int ret=

    bstream_rd_catalogue(&s, static_cast<st_bstream_image_header*>(&info));


This looks nicer in my eyes and is in line with the rules too.

...

Regards
Ingo
-- 
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder,   Wolfgang Engels,   Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring   HRB München 161028

Thread
bzr commit into mysql-6.0-backup branch (Rafal.Somla:2879) Bug#40756Rafal Somla9 Oct
  • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2879)Bug#40756Ingo Strüwing13 Oct
    • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2879)Bug#40756Rafal Somla21 Oct
      • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2879)Bug#40756Konstantin Osipov21 Oct
        • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2879)Bug#40756Rafal Somla22 Oct
      • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2879)Bug#40756Ingo Strüwing22 Oct
        • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2879)Bug#40756Rafal Somla22 Oct