Re: [PATCH 1/3] Refactor get_init_files to use std::string

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:

> To avoid manual memory management.

Thanks!  Comments below.

> Tested on buildbot.
>
> gdb/ChangeLog:
>
> 2019-08-20  Christian Biesinger  <cbiesinger@google.com>
>
> 	* main.c (get_init_files): Change to use std::string.
> 	(captured_main_1): Update.
> 	(print_gdb_help): Update.
> ---
>  gdb/main.c | 96 ++++++++++++++++++++++++++----------------------------
>  1 file changed, 46 insertions(+), 50 deletions(-)
>
> diff --git a/gdb/main.c b/gdb/main.c
> index 678c413021..b9e12589ab 100644
> --- a/gdb/main.c
> +++ b/gdb/main.c
> @@ -195,27 +195,26 @@ relocate_gdb_directory (const char *initial, int flag)
>     return them in SYSTEM_GDBINIT, HOME_GDBINIT, LOCAL_GDBINIT.  If
>     there is no system gdbinit (resp. home gdbinit and local gdbinit)
>     to be loaded, then SYSTEM_GDBINIT (resp. HOME_GDBINIT and
> -   LOCAL_GDBINIT) is set to NULL.  */
> +   LOCAL_GDBINIT) is set to the empty string.  */
>  static void
> -get_init_files (const char **system_gdbinit,
> -		const char **home_gdbinit,
> -		const char **local_gdbinit)
> +get_init_files (std::string *system_gdbinit,
> +		std::string *home_gdbinit,
> +		std::string *local_gdbinit)
>  {
> -  static const char *sysgdbinit = NULL;
> -  static char *homeinit = NULL;
> -  static const char *localinit = NULL;
> +  static std::string sysgdbinit;
> +  static std::string homeinit;
> +  static std::string localinit;
>    static int initialized = 0;
>  
>    if (!initialized)
>      {
>        struct stat homebuf, cwdbuf, s;
> -      const char *homedir;
>  
>        if (SYSTEM_GDBINIT[0])
>  	{
>  	  int datadir_len = strlen (GDB_DATADIR);
>  	  int sys_gdbinit_len = strlen (SYSTEM_GDBINIT);

Since you're cleaning things up, I wouldn't mind converting these two
variables to 'size_t' :-).

> -	  char *relocated_sysgdbinit;
> +	  std::string relocated_sysgdbinit;
>  
>  	  /* If SYSTEM_GDBINIT lives in data-directory, and data-directory
>  	     has been provided, search for SYSTEM_GDBINIT there.  */
> @@ -226,28 +225,30 @@ get_init_files (const char **system_gdbinit,
>  	    {
>  	      /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR
>  		 to gdb_datadir.  */
> -	      char *tmp_sys_gdbinit = xstrdup (&SYSTEM_GDBINIT[datadir_len]);
> -	      char *p;
>  
> -	      for (p = tmp_sys_gdbinit; IS_DIR_SEPARATOR (*p); ++p)
> +	      size_t start = datadir_len;
> +	      for (; IS_DIR_SEPARATOR (SYSTEM_GDBINIT[start]); ++start)
>  		continue;

This seems wrong; you're starting the iteration from
'SYSTEM_GDBINIT[start]', but 'start' is 'datadir_len'.

> -	      relocated_sysgdbinit = concat (gdb_datadir, SLASH_STRING, p,
> -					     (char *) NULL);
> -	      xfree (tmp_sys_gdbinit);
> +	      relocated_sysgdbinit = std::string (gdb_datadir) + SLASH_STRING +
> +		&SYSTEM_GDBINIT[start];
>  	    }
>  	  else
>  	    {
> -	      relocated_sysgdbinit = relocate_path (gdb_program_name,
> -						    SYSTEM_GDBINIT,
> -						    SYSTEM_GDBINIT_RELOCATABLE);
> +	      char *relocated = relocate_path (gdb_program_name,
> +					       SYSTEM_GDBINIT,
> +					       SYSTEM_GDBINIT_RELOCATABLE);
> +	      if (relocated != nullptr)
> +	        {
> +		  relocated_sysgdbinit = relocated;
> +		  xfree (relocated);
> +		}
>  	    }
> -	  if (relocated_sysgdbinit && stat (relocated_sysgdbinit, &s) == 0)
> +	  if (!relocated_sysgdbinit.empty () &&
> +	      stat (relocated_sysgdbinit.c_str (), &s) == 0)
>  	    sysgdbinit = relocated_sysgdbinit;
> -	  else
> -	    xfree (relocated_sysgdbinit);
>  	}
>  
> -      homedir = getenv ("HOME");
> +      const char *homedir = getenv ("HOME");
>  
>        /* If the .gdbinit file in the current directory is the same as
>  	 the $HOME/.gdbinit file, it should not be sourced.  homebuf
> @@ -260,17 +261,16 @@ get_init_files (const char **system_gdbinit,
>  
>        if (homedir)
>  	{
> -	  homeinit = xstrprintf ("%s/%s", homedir, GDBINIT);
> -	  if (stat (homeinit, &homebuf) != 0)
> +	  homeinit = std::string (homedir) + "/" + GDBINIT;

I know the old code did that already, and I also know that SLASH_STRING
is always defined as "/", but I think we should use it here, instead of
hard coding the "/".

> +	  if (stat (homeinit.c_str (), &homebuf) != 0)
>  	    {
> -	      xfree (homeinit);
> -	      homeinit = NULL;
> +	      homeinit = "";
>  	    }
>  	}
>  
>        if (stat (GDBINIT, &cwdbuf) == 0)
>  	{
> -	  if (!homeinit
> +	  if (homeinit.empty ()
>  	      || memcmp ((char *) &homebuf, (char *) &cwdbuf,
>  			 sizeof (struct stat)))
>  	    localinit = GDBINIT;
> @@ -470,11 +470,6 @@ captured_main_1 (struct captured_main_args *context)
>    /* All arguments of --directory option.  */
>    std::vector<char *> dirarg;
>  
> -  /* gdb init files.  */
> -  const char *system_gdbinit;
> -  const char *home_gdbinit;
> -  const char *local_gdbinit;
> -
>    int i;
>    int save_auto_load;
>    int ret = 1;
> @@ -908,6 +903,7 @@ captured_main_1 (struct captured_main_args *context)
>    /* Lookup gdbinit files.  Note that the gdbinit file name may be
>       overriden during file initialization, so get_init_files should be
>       called after gdb_init.  */
> +  std::string system_gdbinit, home_gdbinit, local_gdbinit;

I'd prefer if you kept each variable declared separately, like it was
before:

  std::string system_gdbinit;
  std::string home_gdbinit;
  std::string local_gdbinit;

IIRC, we discourage declaring many variables in the same line.

>    get_init_files (&system_gdbinit, &home_gdbinit, &local_gdbinit);
>  
>    /* Do these (and anything which might call wrap_here or *_filtered)
> @@ -984,16 +980,16 @@ captured_main_1 (struct captured_main_args *context)
>       This is done *before* all the command line arguments are
>       processed; it sets global parameters, which are independent of
>       what file you are debugging or what directory you are in.  */
> -  if (system_gdbinit && !inhibit_gdbinit)
> -    ret = catch_command_errors (source_script, system_gdbinit, 0);
> +  if (!system_gdbinit.empty () && !inhibit_gdbinit)
> +    ret = catch_command_errors (source_script, system_gdbinit.c_str (), 0);
>  
>    /* Read and execute $HOME/.gdbinit file, if it exists.  This is done
>       *before* all the command line arguments are processed; it sets
>       global parameters, which are independent of what file you are
>       debugging or what directory you are in.  */
>  
> -  if (home_gdbinit && !inhibit_gdbinit && !inhibit_home_gdbinit)
> -    ret = catch_command_errors (source_script, home_gdbinit, 0);
> +  if (!home_gdbinit.empty () && !inhibit_gdbinit && !inhibit_home_gdbinit)
> +    ret = catch_command_errors (source_script, home_gdbinit.c_str (), 0);
>  
>    /* Process '-ix' and '-iex' options early.  */
>    for (i = 0; i < cmdarg_vec.size (); i++)
> @@ -1096,20 +1092,20 @@ captured_main_1 (struct captured_main_args *context)
>  
>    /* Read the .gdbinit file in the current directory, *if* it isn't
>       the same as the $HOME/.gdbinit file (it should exist, also).  */
> -  if (local_gdbinit)
> +  if (!local_gdbinit.empty ())
>      {
>        auto_load_local_gdbinit_pathname
> -	= gdb_realpath (local_gdbinit).release ();
> +	= gdb_realpath (local_gdbinit.c_str ()).release ();
>  
>        if (!inhibit_gdbinit && auto_load_local_gdbinit
> -	  && file_is_auto_load_safe (local_gdbinit,
> +	  && file_is_auto_load_safe (local_gdbinit.c_str (),
>  				     _("auto-load: Loading .gdbinit "
>  				       "file \"%s\".\n"),
> -				     local_gdbinit))
> +				     local_gdbinit.c_str ()))
>  	{
>  	  auto_load_local_gdbinit_loaded = 1;
>  
> -	  ret = catch_command_errors (source_script, local_gdbinit, 0);
> +	  ret = catch_command_errors (source_script, local_gdbinit.c_str (), 0);
>  	}
>      }
>  
> @@ -1203,9 +1199,9 @@ gdb_main (struct captured_main_args *args)
>  static void
>  print_gdb_help (struct ui_file *stream)
>  {
> -  const char *system_gdbinit;
> -  const char *home_gdbinit;
> -  const char *local_gdbinit;
> +  std::string system_gdbinit;
> +  std::string home_gdbinit;
> +  std::string local_gdbinit;
>  
>    get_init_files (&system_gdbinit, &home_gdbinit, &local_gdbinit);
>  
> @@ -1283,18 +1279,18 @@ Other options:\n\n\
>    fputs_unfiltered (_("\n\
>  At startup, GDB reads the following init files and executes their commands:\n\
>  "), stream);
> -  if (system_gdbinit)
> +  if (!system_gdbinit.empty ())
>      fprintf_unfiltered (stream, _("\
>     * system-wide init file: %s\n\
> -"), system_gdbinit);
> -  if (home_gdbinit)
> +"), system_gdbinit.c_str ());
> +  if (!home_gdbinit.empty ())
>      fprintf_unfiltered (stream, _("\
>     * user-specific init file: %s\n\
> -"), home_gdbinit);
> -  if (local_gdbinit)
> +"), home_gdbinit.c_str ());
> +  if (!local_gdbinit.empty ())
>      fprintf_unfiltered (stream, _("\
>     * local init file (see also 'set auto-load local-gdbinit'): ./%s\n\
> -"), local_gdbinit);
> +"), local_gdbinit.c_str ());
>    fputs_unfiltered (_("\n\
>  For more information, type \"help\" from within GDB, or consult the\n\
>  GDB manual (available as on-line info or a printed manual).\n\
> -- 
> 2.23.0.rc1.153.gdeed80330f-goog

Otherwise, LGTM.  Thanks for doing this!

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/