Merge lp:~percona-dev/percona-server/no-warnings into lp:percona-server/release-5.1.54-12

[Alexey Kopytov]

[Yasufumi Kinoshita (yasufumi-kinoshita)]

review: Approve

[Yasufumi Kinoshita (yasufumi-kinoshita)]

Hmm?

382 -+ lint n_flush;
383 -+ lint blocks_sum, new_blocks_sum, flushed_blocks_sum;
384 ++ ulint n_flush;
385 ++ ulint blocks_sum;
386 ++ ulint new_blocks_sum;
387 ++ ulint flushed_blocks_sum;

after the change,

n_flush = blocks_sum * (lsn - lsn_old) / log_sys->max_modified_age_async;
if (flushed_blocks_sum > n_pages_flushed_prev) {
        n_flush -= (flushed_blocks_sum - n_pages_flushed_prev);
}

is changed?

n_flush can be minus logically.

[Yasufumi Kinoshita (yasufumi-kinoshita)]

review: Needs Fixing

[Yasufumi Kinoshita (yasufumi-kinoshita)]

Does force decreasing warnings really increase the stability?
It may increase bugs.

[Alexey Kopytov (akopytov)]

Yasufumi,

Thanks for your review.

> Hmm?
>
> 382 -+ lint n_flush;
> 383 -+ lint blocks_sum, new_blocks_sum, flushed_blocks_sum;
> 384 ++ ulint n_flush;
> 385 ++ ulint blocks_sum;
> 386 ++ ulint new_blocks_sum;
> 387 ++ ulint flushed_blocks_sum;
>
> after the change,
>
> n_flush = blocks_sum * (lsn - lsn_old) / log_sys->max_modified_age_async;
> if (flushed_blocks_sum > n_pages_flushed_prev) {
> n_flush -= (flushed_blocks_sum - n_pages_flushed_prev);
> }
>
> is changed?
>
> n_flush can be minus logically.

Good catch! I have fixed that.

[Alexey Kopytov (akopytov)]

> Does force decreasing warnings really increase the stability?

Yes, it does increase the code quality. Quite often a compiler warning indicates a real problem/bug in the code. But if you have many warnings, it will go unnoticed. The only way to make sure all warnings are addressed by developers is to fix all existing warnings and make sure no new ones are introduced.

Upstream MySQL development has done a huge work to remove warnings. As a part of that work, --with-debug=full now builds -Werror to make sure no new warnings are introduced. That, however, prevented Percona Server to be built with -Werror.

> It may increase bugs.

I don't see any possible scenarios where a removing compiler warning would directly lead to new bugs.
Can you elaborate on that?

[Yasufumi Kinoshita (yasufumi-kinoshita)]

I mean it should be done by the person who know the code well.

In this case, you may break logic of the exist code.
Simple decrement of the warning without knowledge may cause broken code.

If you don't change the sign of the types, it works fine 100% as intended by me.

I always aware all workloads all over the world.
Even if it is very rare case, I should be consider.

So, the warning decrement activity should be done by the author ideally,
if you still insist it should be done.

[Alexey Kopytov (akopytov)]

Yasufumi,

As long as I am able to build Percona Server with -Werror, I'm happy. If you think you can fix that yourself, that's fine, just let me know when you are going to do that.

[Yasufumi Kinoshita (yasufumi-kinoshita)]

Alexey,

I used "-Wall -Wpointer-arith" and confirmed their safeness. (added "-Wpointer-arith" in this half of year.)

Why don't you explain the merit to reduce the warnings (not only confirm them)
for the code based on objective facts and logic....?

And I think you should never get mysqld with "-Werror" at many of distributions...
I always get warnings from basic mysql code with my options.
And the older GCC 4.1.? (CentOS5.5) always print warning about atomic-builtin of gcc...

Does removing "-Wall -Wpointer-arith" and using "-Werror" increase quality of code?

Or, should I also need to build with "-Werror" without "-Wall -Wpointer-arith" for each development and for your happiness?

I am happy, if the possibility to contort or break our codes are eliminated.
It seemed for me that you introduced the possibility only for your preference.

Thanks.

[Oleg Tsarev (tsarev)]

All good for me

review: Approve

[Yasufumi Kinoshita (yasufumi-kinoshita)]

I suspect your are really careless.

"-+ struct timespec tp;"
is absolutely wrong.

Though I can imagine the warning you intended,
you seems not to build Percona-Server correctly.

I was intended to cause warning when HAVE_CLOCK_GETTIME is not defined
not to build as not intended.
But the warning code seems to be removed by someone.... :-(
Who did remove the code from userstat.patch without my review?? you?

If the feature is not included to our build of Linux, it is bug.
The removing warning message may cause the bug!

"reducing warnings by author" may reduce bugs.
But "reducing warnings by you" may increase bugs, in this case.

The warning message should be... "at linux, 'export LIBS=-lrt' for configure."

Anyway the code should be.
+#ifdef HAVE_CLOCK_GETTIME
    struct timespec tp;
+#endif

Why you cannot read the just after the definition using #ifdef HAVE_CLOCK_GETTIME ?

I think you should not touch existing code at all.
You seems not to read the code.

I don't believe your editing basically. It seems only increase my work.

I will fix them by myself about my patches ("maintainer : Yasufumi" in the header).

########################################################################
Please list warnings and make bug report, instead of editing directly.
########################################################################

I think you should not edit, too careless.

Please remove fix for userstat.patch from the merge also, I will do by myself.

But for show_patches.patch is OK for now.

review: Needs Fixing

[Alexey Kopytov (akopytov)]

Yasufumi,

Thanks for your review. I remember checking if "tp" is actually used and reverting the change, but I probably forgot to update the patch after reverting those changes in the source tree. Anyway, I have now reverted all changes to userstat.patch.

Regarding the warnings about "-lrt", they were removed by me and you were one of the reviewers for the patch.

Please let me know if you see any other problems with the current version of the changes.

[Yasufumi Kinoshita (yasufumi-kinoshita)]

Thank you.
In the end... no part to be review by me...

>> "-lrt"
OK... I am confused a little...

But I am struggling with the rest of warnings now.
Please looking forward to the my proposal to merge.

review: Approve

[Alexey Kopytov (akopytov)]

Approved by both reviewers, changing the status to Approved

=== modified file 'error_pad.patch'
--- error_pad.patch	2010-10-19 00:20:32 +0000
+++ error_pad.patch	2011-02-02 08:13:28 +0000
@@ -61,7 +61,7 @@
 -	goto err;
 +        int padd_to= tmp_error->d_code;
 +        char* padd_message= tmp->text;
-+        while ((row_nr+er_offset) < padd_to)
++        while ((int) row_nr + er_offset < padd_to)
 +        {
 +          if (copy_rows(to, padd_message,row_nr,start_pos))
 +          {
@@ -177,7 +177,7 @@
 +    fprintf(stderr, "Failed to parse the error padd string '%s' '%s' (d_code doesn't parse)!\n",er_name,str);
 +    DBUG_RETURN(0);
 +  }
-+  if (d_code < (er_offset + er_count))
++  if (d_code < (uint) er_offset + er_count)
 +  {
 +    fprintf(stderr, "Error to padding less current error number!\n");
 +    DBUG_RETURN(0);

=== modified file 'response-time-distribution.patch'
--- response-time-distribution.patch	2011-01-10 13:38:39 +0000
+++ response-time-distribution.patch	2011-02-02 08:13:28 +0000
@@ -369,7 +369,7 @@
 +  buffer[string_positive_power_length]= '.';
 +  ulonglong second=      (value / MILLION);
 +  ulonglong microsecond= (value % MILLION);
-+  std::size_t result_length= snprintf(buffer,buffer_size,format,second,microsecond);
++  int result_length= snprintf(buffer, buffer_size, format, second, microsecond);
 +  if(result_length < 0)
 +  {
 +    assert(sizeof(STRING_OVERFLOW) <= buffer_size);
@@ -514,7 +514,7 @@
 +    DBUG_ENTER("fill_schema_query_response_time");
 +    TABLE        *table= static_cast<TABLE*>(tables->table);
 +    Field        **fields= table->field;
-+    for(int i= 0, count= bound_count() + 1 /* with overflow */; count > i; ++i)
++    for(uint i= 0, count= bound_count() + 1 /* with overflow */; count > i; ++i)
 +    {
 +      char time[TIME_STRING_BUFFER_LENGTH];
 +      char total[TOTAL_STRING_BUFFER_LENGTH];

=== modified file 'show_patches.patch'
--- show_patches.patch	2010-12-16 11:35:26 +0000
+++ show_patches.patch	2011-02-02 08:13:28 +0000
@@ -160,7 +160,7 @@
 +	const char *comment;
 +}patches[] = {
 +$output
-+{NULL, NULL, NULL, NULL}
++{NULL, NULL, NULL, NULL, NULL, NULL}
 +};	
 +
 +HEADER

=== modified file 'sql_no_fcache.patch'
--- sql_no_fcache.patch	2010-12-17 19:06:49 +0000
+++ sql_no_fcache.patch	2011-02-02 08:13:28 +0000
@@ -358,11 +358,13 @@
  const char *join_type_str[]={ "UNKNOWN","system","const","eq_ref","ref",
  			      "MAYBE_REF","ALL","range","index","fulltext",
  			      "ref_or_null","unique_subquery","index_subquery",
-@@ -239,9 +245,18 @@
+@@ -239,9 +245,20 @@
                     ulong setup_tables_done_option)
  {
    bool res;
++#if defined(__linux__)
 +  pid_t pid;
++#endif
    register SELECT_LEX *select_lex = &lex->select_lex;
    DBUG_ENTER("handle_select");