maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Sergei,

thanks for your feedback. I implemented all your suggestions.

Regards,
Sergey

On Wed, Nov 13, 2013 at 12:58:28PM +0100, Sergei Golubchik wrote:
> 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