From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 87325 invoked by alias); 21 Aug 2019 17:13:48 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 87317 invoked by uid 89); 21 Aug 2019 17:13:48 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-15.6 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 21 Aug 2019 17:13:46 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0443E8980F6; Wed, 21 Aug 2019 17:13:45 +0000 (UTC) Received: from localhost (unused-10-15-17-196.yyz.redhat.com [10.15.17.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id C13475C1D6; Wed, 21 Aug 2019 17:13:44 +0000 (UTC) From: Sergio Durigan Junior To: "Christian Biesinger via gdb-patches" Cc: Christian Biesinger Subject: Re: [PATCH 1/3] Refactor get_init_files to use std::string References: <20190820221745.147370-1-cbiesinger@google.com> <20190820221745.147370-2-cbiesinger@google.com> Date: Wed, 21 Aug 2019 17:13:00 -0000 In-Reply-To: <20190820221745.147370-2-cbiesinger@google.com> (Christian Biesinger via gdb-patches's message of "Tue, 20 Aug 2019 17:17:43 -0500") Message-ID: <87v9uqjvav.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2019-08/txt/msg00490.txt.bz2 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 > > * 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 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/