maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sergey!

Generally, it's ok, of course.
But as a matter of style (up to you to fix it here, but keep in mind for
the future) - see below

On Nov 13, Sergey Vojtovich wrote:
> === modified file 'sql/mysqld.cc'
> --- a/sql/mysqld.cc	2013-11-01 11:00:11 +0000
> +++ b/sql/mysqld.cc	2013-11-13 07:07:55 +0000
> @@ -760,6 +760,7 @@ char **orig_argv;
>  
>  static struct my_option pfs_early_options[] __attribute__((unused)) =

I suppose, we don't need __attribute__((unused)) here, if the array is
used unconditionally.

>  {
> +#ifdef WITH_PERFSCHEMA_STORAGE_ENGINE
>    {"performance_schema_instrument", OPT_PFS_INSTRUMENT,
>      "Default startup value for a performance schema instrument.",
>      &pfs_param.m_pfs_instrument, &pfs_param.m_pfs_instrument, 0, GET_STR,
> @@ -824,6 +825,7 @@ static struct my_option pfs_early_option
>      &pfs_param.m_consumer_statement_digest_enabled,
>      &pfs_param.m_consumer_statement_digest_enabled, 0,
>      GET_BOOL, OPT_ARG, TRUE, 0, 0, 0, 0, 0}
> +#endif /* WITH_PERFSCHEMA_STORAGE_ENGINE */
>  };
>  
>  #ifdef HAVE_PSI_INTERFACE
> @@ -1146,7 +1148,6 @@ void net_after_header_psi(struct st_net
>  
>  void init_net_server_extension(THD *thd)
>  {
> -#ifdef HAVE_PSI_INTERFACE
>    /* Start with a clean state for connection events. */
>    thd->m_idle_psi= NULL;
>    thd->m_statement_psi= NULL;
> @@ -1157,9 +1158,6 @@ void init_net_server_extension(THD *thd)
>    thd->m_net_server_extension.m_after_header= net_after_header_psi;
>    /* Activate this private extension for the mysqld server. */
>    thd->net.extension= & thd->m_net_server_extension;
> -#else
> -  thd->net.extension= NULL;
> -#endif
>  }
>  #endif /* EMBEDDED_LIBRARY */
>  
> @@ -4397,7 +4395,9 @@ static int init_thread_environment()
>  #ifdef HAVE_EVENT_SCHEDULER
>    Events::init_mutexes();
>  #endif
> +#ifdef HAVE_PSI_INTERFACE
>    init_show_explain_psi_keys();
> +#endif

I usually define an no-op function and use it unconditionally, to avoid
scattering #ifdefs in the code.

>    /* Parameter for threads created for connections */
>    (void) pthread_attr_init(&connection_attrib);
>    (void) pthread_attr_setdetachstate(&connection_attrib,
> @@ -5137,9 +5137,12 @@ int mysqld_main(int argc, char **argv)
>    buffered_logs.init();
>    my_getopt_error_reporter= buffered_option_error_reporter;
>    my_charset_error_reporter= buffered_option_error_reporter;
> +#ifdef WITH_PERFSCHEMA_STORAGE_ENGINE
>    pfs_param.m_pfs_instrument= const_cast<char*>("");
>  
> -  int ho_error= handle_early_options();
> +  int ho_error=
> +#endif /* WITH_PERFSCHEMA_STORAGE_ENGINE */
> +    handle_early_options();

Please, don't #ifdef part of a statement. Here I'd use
__attribute__((unused)) to suppress a compiler warning.

>  #ifdef WITH_PERFSCHEMA_STORAGE_ENGINE
>    if (ho_error == 0)
> 
Regards,
Sergei