From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 54166 invoked by alias); 21 Aug 2019 17:44:37 -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 54113 invoked by uid 89); 21 Aug 2019 17:44:36 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-29.3 required=5.0 tests=AWL,BAYES_00,ENV_AND_HDR_SPF_MATCH,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,RCVD_IN_JMF_BL,SPF_PASS,USER_IN_DEF_SPF_WL autolearn=ham version=3.3.1 spammy=HX-HELO:sk:mail-oi, HX-Received:aca, H*RU:209.85.167.193, HX-Spam-Relays-External:209.85.167.193 X-HELO: mail-oi1-f193.google.com Received: from mail-oi1-f193.google.com (HELO mail-oi1-f193.google.com) (209.85.167.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 21 Aug 2019 17:44:35 +0000 Received: by mail-oi1-f193.google.com with SMTP id h21so2238979oie.7 for ; Wed, 21 Aug 2019 10:44:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=P63SYLNd2TZ/vZIV8dpBDI/I7oPev5rCqDApmHpretk=; b=Pcp1M1+zeCiFQJILsn+jSyq1aJnOtU8QWwTIfvI/vZywkNkoD+RgAd3dhNLGsclF0v jP9fts37VwrMiFDKQILyIYgjIkEQleGTjeHD7SROWXV981ClKVf/oqGbM09CXwlQX2f9 mha0biLMNSfLqWnAoyrfHFmH0GjiKCIvfGHW+OcTdISUjA/TodgJIubvogAe4c4n2SK4 vBSO/7P8GZLLyG1V/YEnTcfge9vqFMERi5dEKD44Kf3T8Vjcqyw+tXWbDpJVk/DFs7uC o1Lc1EddlVFDYhn/7a/RWugUxY+7ArfgyYHQOsUaIWWnmOmvktOWzcQoxEsbTtaqQmpP 5m5w== MIME-Version: 1.0 References: <20190820221745.147370-1-cbiesinger@google.com> <20190820221745.147370-3-cbiesinger@google.com> <87r25ejv1r.fsf@redhat.com> In-Reply-To: <87r25ejv1r.fsf@redhat.com> From: "Christian Biesinger via gdb-patches" Reply-To: Christian Biesinger Date: Wed, 21 Aug 2019 17:44:00 -0000 Message-ID: Subject: Re: [PATCH 2/3] Factor out the code to do the datadir-relocation for gdbinit To: Sergio Durigan Junior Cc: Christian Biesinger via gdb-patches Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-08/txt/msg00497.txt.bz2 On Wed, Aug 21, 2019 at 12:19 PM Sergio Durigan Junior wrote: > > On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote: > > > gdb/ChangeLog: > > > > 2019-08-20 Christian Biesinger > > > > * main.c (relocate_gdbinit_path_maybe_in_datadir): New function. > > (get_init_files): Update. > > I'm afraid you'll need a descriptive commit message :-). Changed to: 2019-08-20 Christian Biesinger * main.c (relocate_gdbinit_path_maybe_in_datadir): Factor this code out of get_init_files. (get_init_files): Update. (I guess the title of the commit message is not enough?) > > --- > > gdb/main.c | 68 +++++++++++++++++++++++++++++------------------------- > > 1 file changed, 37 insertions(+), 31 deletions(-) > > > > diff --git a/gdb/main.c b/gdb/main.c > > index b9e12589ab..a1d1904c9b 100644 > > --- a/gdb/main.c > > +++ b/gdb/main.c > > @@ -191,6 +191,41 @@ relocate_gdb_directory (const char *initial, int flag) > > return dir; > > } > > > > +static std::string relocate_gdbinit_path_maybe_in_datadir (std::string file) > > You should break the line after 'std::string': > > static std::string > relocate_gdbinit_path_maybe_in_datadir (std::string file) Thanks, done and also changed std::string to const std::string&. > > +{ > > + int datadir_len = strlen (GDB_DATADIR); > > size_t. > > Also, you could declare a return variable here and just fill it > inside each 'if', instead of returning early (and then having to return > an empty string at the end (but that's a matter of style, I know). OK, if you prefer, sure. Done. > > + > > + /* If SYSTEM_GDBINIT lives in data-directory, and data-directory > > + has been provided, search for SYSTEM_GDBINIT there. */ > > + if (gdb_datadir_provided > > + && datadir_len < file.length () > > + && filename_ncmp (file.c_str (), GDB_DATADIR, datadir_len) == 0 > > + && IS_DIR_SEPARATOR (file[datadir_len])) > > + { > > + /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR > > + to gdb_datadir. */ > > + > > + size_t start = datadir_len; > > + for (; IS_DIR_SEPARATOR (file[start]); ++start) > > + continue; > > Same comment here: this loop seems strange (starting from 'start'). See my response in the other thread. > > + return std::string (gdb_datadir) + SLASH_STRING + > > + file.substr(start); > > + } > > + else > > + { > > + char *relocated = relocate_path (gdb_program_name, > > + file.c_str(), > > + SYSTEM_GDBINIT_RELOCATABLE); > > + if (relocated != nullptr) > > + { > > + std::string retval(relocated); > > Space between variable name and open parenthesis. Thanks! (Though irrelevant with the other change now) > > + xfree (relocated); > > + return retval; > > + } > > + } > > + return ""; > > +} > > + > > /* Compute the locations of init files that GDB should source and > > return them in SYSTEM_GDBINIT, HOME_GDBINIT, LOCAL_GDBINIT. If > > there is no system gdbinit (resp. home gdbinit and local gdbinit) > > @@ -212,37 +247,8 @@ get_init_files (std::string *system_gdbinit, > > > > if (SYSTEM_GDBINIT[0]) > > { > > - int datadir_len = strlen (GDB_DATADIR); > > - int sys_gdbinit_len = strlen (SYSTEM_GDBINIT); > > - std::string relocated_sysgdbinit; > > - > > - /* If SYSTEM_GDBINIT lives in data-directory, and data-directory > > - has been provided, search for SYSTEM_GDBINIT there. */ > > - if (gdb_datadir_provided > > - && datadir_len < sys_gdbinit_len > > - && filename_ncmp (SYSTEM_GDBINIT, GDB_DATADIR, datadir_len) == 0 > > - && IS_DIR_SEPARATOR (SYSTEM_GDBINIT[datadir_len])) > > - { > > - /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR > > - to gdb_datadir. */ > > - > > - size_t start = datadir_len; > > - for (; IS_DIR_SEPARATOR (SYSTEM_GDBINIT[start]); ++start) > > - continue; > > - relocated_sysgdbinit = std::string (gdb_datadir) + SLASH_STRING + > > - &SYSTEM_GDBINIT[start]; > > - } > > - else > > - { > > - char *relocated = relocate_path (gdb_program_name, > > - SYSTEM_GDBINIT, > > - SYSTEM_GDBINIT_RELOCATABLE); > > - if (relocated != nullptr) > > - { > > - relocated_sysgdbinit = relocated; > > - xfree (relocated); > > - } > > - } > > + std::string relocated_sysgdbinit = > > + relocate_gdbinit_path_maybe_in_datadir (SYSTEM_GDBINIT); > > if (!relocated_sysgdbinit.empty () && > > stat (relocated_sysgdbinit.c_str (), &s) == 0) > > sysgdbinit = relocated_sysgdbinit; > > -- > > 2.23.0.rc1.153.gdeed80330f-goog > > Otherwise, LGTM. Thanks. Christian