List:Commits« Previous MessageNext Message »
From:Libing Song Date:December 22 2010 10:43am
Subject:Re: bzr commit into mysql-5.5-bugteam branch (zhenxing.he:3180)
Bug#57953
View as plain text  
Great work.
Please find my review comments below.

STATUS
------
 
  Approved.

REQUIRED CHANGES
----------------
No

REQUESTS
--------
No

SUGGESTIONS
-----------
SU1. Could you please modify the code to fulfill the coding guideline.
     Please check the detail comments.



DETAILS 
-------
> === modified file 'extra/my_print_defaults.c'
> --- a/extra/my_print_defaults.c	2010-07-15 11:13:30 +0000
> +++ b/extra/my_print_defaults.c	2010-12-20 04:30:59 +0000
> @@ -193,7 +193,7 @@ int main(int argc, char **argv)
>    }
>  
>    for (argument= arguments+1 ; *argument ; argument++)
> -    if (*argument != args_separator)           /* skip arguments separator */
> +    if (!my_getopt_is_args_separator(*argument))           /* skip arguments
> separator */
The line is too long. Please make it short than 80 chars.
>     puts(*argument);
>    my_free(load_default_groups);
>    free_defaults(arguments);
> 
[snip]
> @@ -515,17 +530,28 @@ int my_load_defaults(const char *conf_fi
>    if (*argc >= 2 && !strcmp(argv[0][1],"--no-defaults"))
>    {
>      /* remove the --no-defaults argument and return only the other arguments */
> -    uint i;
> +    uint i, j;
>      if (!(ptr=(char*) alloc_root(&alloc,sizeof(alloc)+
>  				 (*argc + 1)*sizeof(char*))))
>        goto err;
>      res= (char**) (ptr+sizeof(alloc));
>      res[0]= **argv;				/* Copy program name */
> -    /* set arguments separator */
> -    res[1]= (char *)args_separator;
> -    for (i=2 ; i < (uint) *argc ; i++)
> -      res[i]=argv[0][i];
> -    res[i]=0;					/* End pointer */
> +    j= 1;                 /* Start from 1 for the reset result args */
> +    if (my_getopt_use_args_separator)
> +    {
> +      /* set arguments separator */
> +      set_args_separator(&res[1]);
> +      j++;
> +    }
> +    for (i=2 ; i < (uint) *argc ; i++, j++)
> +      res[j]=argv[0][i];
> +    res[j]=0;	
Please add a space after '='.
> 				/* End pointer */

[snip]
> @@ -580,16 +606,19 @@ int my_load_defaults(const char *conf_fi
>      --*argc; ++*argv;				/* skip argument */
>    }
>  
> -  /* set arguments separator for arguments from config file and
> -     command line */
> -  res[args.elements+1]= (char *)args_separator;
> +  if (my_getopt_use_args_separator)
> +  {
> +    /* set arguments separator for arguments from config file and
> +       command line */
> +    set_args_separator(&res[args.elements+1]);
> +  }
>  
>    if (*argc)
> -    memcpy((uchar*) (res+1+args.elements+1), (char*) ((*argv)+1),
> +    memcpy((uchar*) (res+1+args.elements+args_sep), (char*) ((*argv)+1),
>  	   (*argc-1)*sizeof(char*));
> -  res[args.elements+ *argc+1]=0;                /* last null */
> +  res[args.elements+ *argc+args_sep]=0;                /* last null */
>  
Please add a space after '='.
> -  (*argc)+=args.elements+1;
> +  (*argc)+=args.elements+args_sep;
Same to above.
>    *argv= (char**) res;
>    *(MEM_ROOT*) ptr= alloc;			/* Save alloc root for free */
>    delete_dynamic(&args);
> @@ -599,7 +628,7 @@ int my_load_defaults(const char *conf_fi
>      printf("%s would have been started with the following arguments:\n",
>  	   **argv);
>      for (i=1 ; i < *argc ; i++)
> -      if ((*argv)[i] != args_separator) /* skip arguments separator */
> +      if (!my_getopt_is_args_separator((*argv)[i])) /* skip arguments separator */
>          printf("%s ", (*argv)[i]);
>      puts("");
>      exit(0);
> 

-- 
Your Sincerely,
Libing Song
==================================
MySQL Replication Team
Software Engineer


Email : Li-Bing.Song@stripped
Skype : libing.song
MSN   : slb_database@stripped
Phone : +86 010-6505-4020 ext. 319
Mobile: +86 138-1144-2038
==================================

Thread
bzr commit into mysql-5.5-bugteam branch (zhenxing.he:3180) Bug#57953He Zhenxing20 Dec
  • Re: bzr commit into mysql-5.5-bugteam branch (zhenxing.he:3180)Bug#57953Libing Song22 Dec
  • Re: bzr commit into mysql-5.5-bugteam branch (zhenxing.he:3180) Bug#57953Sven Sandberg14 Jan