List:Commits« Previous MessageNext Message »
From:Tor Didriksen Date:December 20 2010 2:02pm
Subject:Re: bzr commit into mysql-5.5-bugteam branch (tor.didriksen:3195) Bug#58699
View as plain text  
On Thu, Dec 16, 2010 at 1:07 AM, Davi Arnaut <davi.arnaut@stripped> wrote:

> Hi Tor,
>
>
> On 12/15/10 6:32 AM, Tor Didriksen wrote:
>
>> #At file:///export/home/didrik/repo/5.5-bugteam-bug58137/ based on
>> revid:gleb.shchepa@stripped
>>
>>  3195 Tor Didriksen    2010-12-13
>>       Bug #58699 cannot build with gcc dbg on solaris
>>      @ include/my_pthread.h
>>         On solaris pthread_once_t contains an array, so we must wrap the
>> initialization in {}
>>      @ include/mysql/psi/mysql_file.h
>>         Include my_global.h first, to get correct platform definitions.
>>      @ mysys/ptr_cmp.c
>>         Hide the unused static functions in #ifdef's on solaris.
>>      @ sql/my_decimal.cc
>>         Include my_global.h first, to get correct platform definitions.
>>      @ sql/mysqld.cc
>>         Fix signed/unsigned comparison warning.
>>      @ sql/sql_audit.h
>>         Include my_global.h first, to get correct platform definitions.
>>      @ sql/sql_plugin.h
>>         Include my_global.h first, to get correct platform definitions.
>>      @ sql/sql_show.cc
>>         Fix: warning: cast from pointer to integer of different size
>>      @ sql/sys_vars.h
>>         Use reinterpret_cast rather than c-style cast.
>>      @ storage/perfschema/pfs_instr.cc
>>         Include my_global.h first, to get correct platform definitions.
>>
>
> Some comments below.
>
>
>      modified:
>>       include/my_pthread.h
>>       include/mysql/psi/mysql_file.h
>>       mysys/ptr_cmp.c
>>       sql/my_decimal.cc
>>       sql/mysqld.cc
>>       sql/sql_audit.h
>>       sql/sql_plugin.h
>>       sql/sql_show.cc
>>       sql/sys_vars.h
>>       storage/perfschema/pfs_instr.cc
>> === modified file 'include/my_pthread.h'
>> --- a/include/my_pthread.h      2010-10-20 14:48:19 +0000
>> +++ b/include/my_pthread.h      2010-12-13 16:04:03 +0000
>> @@ -214,7 +214,11 @@ int pthread_cancel(pthread_t thread);
>>  typedef void *(* pthread_handler)(void *);
>>
>>  #define my_pthread_once_t pthread_once_t
>> +#if defined __GNUC__ &&  defined TARGET_OS_SOLARIS
>>
>
> defined(__GNUC__) && defined(TARGET_OS_SOLARIS) as used in other places in
> this file. I believe you told me something very similar in a review this
> week :-)
>
>
>  +#define MY_PTHREAD_ONCE_INIT { PTHREAD_ONCE_INIT }
>>
>
> Please reference, in the per-file comment, the bug report about this:
>
> http://bugs.opensolaris.org/bugdatabase/printableBug.do?bug_id=6611808
>
> Also, it seems to me that this is something we should detect at cmake time,
> or tie it to a set of specific Solaris versions (8 and 9). A test at cmake
> time for Solaris 8 and 9 to ensure that { PTHREAD_ONCE_INIT } compiles is
> one option.


yep, a cmake test is much better.


>
>
>  +#else
>>  #define MY_PTHREAD_ONCE_INIT PTHREAD_ONCE_INIT
>> +#endif
>>  #define my_pthread_once(C,F) pthread_once(C,F)
>>
>>  /* Test first for RTS or FSU threads */
>>
>> === modified file 'include/mysql/psi/mysql_file.h'
>> --- a/include/mysql/psi/mysql_file.h    2010-07-09 23:00:24 +0000
>> +++ b/include/mysql/psi/mysql_file.h    2010-12-13 16:04:03 +0000
>> @@ -16,6 +16,8 @@
>>  #ifndef MYSQL_FILE_H
>>  #define MYSQL_FILE_H
>>
>> +#include "my_global.h"
>>
>
> <my_global.h>
>
> Just a style preference in line with the other includes in the file. I
> wonder if there are any practical implications for MySQL code (other then
> the normal difference).


hmm, in C code we seem to do <my_global.h>, in C++ code we seem to do
"my_global.h"
maybe <> is best, indicating that the header file is not local to the
current directory..


>
>
>  +
>>  /* For strlen() */
>>  #include<string.h>
>>  /* For MY_STAT */
>>
>> === modified file 'mysys/ptr_cmp.c'
>> --- a/mysys/ptr_cmp.c   2009-11-20 19:01:43 +0000
>> +++ b/mysys/ptr_cmp.c   2010-12-13 16:04:03 +0000
>> @@ -50,11 +50,15 @@ static int ptr_compare_3(size_t *compare
>>
>>        /* Get a pointer to a optimal byte-compare function for a given
>> size */
>>
>> +#ifdef TARGET_OS_SOLARIS
>>  qsort2_cmp get_ptr_compare (size_t size)
>>  {
>> -#ifdef TARGET_OS_SOLARIS
>> +  (void) size;
>>
>
> Use the unused attribute tag. It can be used even when the argument is
> being used.
>

OK


>
> Also, let's get rid of TARGET_OS_SOLARIS? It's only used in a few places
> and we should actually be using a macro provided by Solaris.
>
>
TARGET_OS_SOLARIS is set by cmake, so it's not so bad to depend on it I
think.
gcc seems to generate these relevant macros: (touch foo.h; cpp -dM foo.h)
#define __sun 1
#define sun 1
#define __sun__ 1

The man page for SunPro cc, lists:
                    __BUILTIN_VA_ARG_INCR
                   __SUNPRO_C=0x590
                   __SVR4(SPARC)
                   __SunOS(Solaris)
                   __amd64(x86 with-m64)
                   __gnu__linux(linux)
                   __i386(x86)
                   __linux(linux)
                   __linux__(linux)
                   __sparc(SPARC)
                   __sparcv9(with-m64)
                   __sun(Solaris)
                   __unix
                   __`uname -s`_`uname -r`(Solaris)
                   __x86_64(x86)
                   linux(x86,linux)

Would you prefer __sun rather than TARGET_OS_SOLARIS ?


>     return (qsort2_cmp) native_compare;
>> +}
>>  #else
>> +qsort2_cmp get_ptr_compare (size_t size)
>> +{
>>    if (size<  4)
>>      return (qsort2_cmp) ptr_compare;
>>    switch (size&  3) {
>> @@ -64,8 +68,8 @@ qsort2_cmp get_ptr_compare (size_t size)
>>      case 3: return (qsort2_cmp) ptr_compare_3;
>>      }
>>    return 0;                                   /* Impossible */
>> -#endif /* TARGET_OS_SOLARIS */
>>  }
>> +#endif /* TARGET_OS_SOLARIS */
>>
>>
>>        /*
>> @@ -75,6 +79,8 @@ qsort2_cmp get_ptr_compare (size_t size)
>>
>>  #define cmp(N) if (first[N] != last[N]) return (int) first[N] - (int)
>> last[N]
>>
>> +#ifndef TARGET_OS_SOLARIS
>> +
>>  static int ptr_compare(size_t *compare_length, uchar **a, uchar **b)
>>  {
>>    reg3 int length= *compare_length;
>> @@ -177,6 +183,8 @@ static int ptr_compare_3(size_t *compare
>>    return (0);
>>  }
>>
>> +#endif /* !TARGET_OS_SOLARIS */
>> +
>>  void my_store_ptr(uchar *buff, size_t pack_length, my_off_t pos)
>>  {
>>    switch (pack_length) {
>>
>> === modified file 'sql/my_decimal.cc'
>> --- a/sql/my_decimal.cc 2010-12-14 16:26:18 +0000
>> +++ b/sql/my_decimal.cc 2010-12-13 16:04:03 +0000
>> @@ -13,6 +13,7 @@
>>     along with this program; if not, write to the Free Software
>>     Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
>>  USA */
>>
>> +#include "my_global.h"
>>  #include "sql_priv.h"
>>  #include<time.h>
>>
>>
>> === modified file 'sql/mysqld.cc'
>> --- a/sql/mysqld.cc     2010-12-14 14:34:23 +0000
>> +++ b/sql/mysqld.cc     2010-12-13 16:04:03 +0000
>> @@ -3258,8 +3258,8 @@ static int init_common_variables()
>>       size_t *pagesize = (size_t *) malloc(sizeof(size_t) * nelem);
>>       if (pagesize != NULL&&  getpagesizes(pagesize, nelem)>  0)
>>       {
>> -       size_t i, max_page_size= 0;
>> -       for (i= 0; i<  nelem; i++)
>> +       size_t max_page_size= 0;
>> +       for (int i= 0; i<  nelem; i++)
>>
>
> This is prohibited in the CodingStyle because some of old compilers in
> platforms we support for 5.1. Need to double check if we support those
> platforms for 5.5. I also vaguely remember that this might have been changed
> in some CodingStyle meeting.. but I'm not sure.
>
>
This has always worked in C++
The problem was with old versions of visual studio which did not limit the
scope of the local variable to the for loop,
it would be visible after the loop.
So you could not do

for (int i= 0; ...) {}
for (int i= 0; ...) {}

the second one would give a double declaration, and this is mentioned in the
style guide as non-portable.

There are no more 'int i' declarations in the enclosing scope, so this
should be safe
(even if you want to compile it with visual studio 5 :-)



>
>          {
>>           if (pagesize[i]>  max_page_size&&
>>               pagesize[i]<= max_desired_page_size)
>>
>> === modified file 'sql/sql_audit.h'
>> --- a/sql/sql_audit.h   2010-12-14 14:34:23 +0000
>> +++ b/sql/sql_audit.h   2010-12-13 16:04:03 +0000
>> @@ -17,6 +17,8 @@
>>     Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
>>  USA */
>>
>>
>> +#include "my_global.h"
>> +
>>  #include<mysql/plugin_audit.h>
>>  #include "sql_class.h"
>>
>>
>> === modified file 'sql/sql_plugin.h'
>> --- a/sql/sql_plugin.h  2010-12-01 22:15:14 +0000
>> +++ b/sql/sql_plugin.h  2010-12-13 16:04:03 +0000
>> @@ -16,6 +16,8 @@
>>  #ifndef _sql_plugin_h
>>  #define _sql_plugin_h
>>
>> +#include "my_global.h"
>> +
>>  /*
>>    the following #define adds server-only members to enum_mysql_show_type,
>>    that is defined in plugin.h
>>
>> === modified file 'sql/sql_show.cc'
>> --- a/sql/sql_show.cc   2010-12-14 10:46:00 +0000
>> +++ b/sql/sql_show.cc   2010-12-13 16:04:03 +0000
>> @@ -2246,7 +2246,7 @@ static bool show_status_array(THD *thd,
>>            end= int10_to_str(*(long*) value, buff, 10);
>>            break;
>>          case SHOW_LONGLONG_STATUS:
>> -          value= ((char *) status_var + (ulonglong) value);
>> +          value= ((char *) status_var + (ulong) value);
>>            /* fall through */
>>          case SHOW_LONGLONG:
>>            end= longlong10_to_str(*(longlong*) value, buff, 10);
>>
>>
> Otherwise, looks good.
>
> Regards,
>
> Davi
>
>
thanks -- didrik


> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:
> http://lists.mysql.com/commits?unsub=1
>
>

Thread
bzr commit into mysql-5.5-bugteam branch (tor.didriksen:3195) Bug#58699Tor Didriksen15 Dec
  • Re: bzr commit into mysql-5.5-bugteam branch (tor.didriksen:3195)Bug#58699Davi Arnaut16 Dec
    • Re: bzr commit into mysql-5.5-bugteam branch (tor.didriksen:3195) Bug#58699Tor Didriksen20 Dec