From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 129506 invoked by alias); 2 Dec 2016 13:05:25 -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 129469 invoked by uid 89); 2 Dec 2016 13:05:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.8 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=submission 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; Fri, 02 Dec 2016 13:05:23 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 348646264E; Fri, 2 Dec 2016 13:05:22 +0000 (UTC) Received: from [127.0.0.1] (ovpn03.gateway.prod.ext.phx2.redhat.com [10.5.9.3]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uB2D5LCC000908; Fri, 2 Dec 2016 08:05:21 -0500 Subject: Re: [RFA 2/8] Use class to manage BFD reference counts To: Tom Tromey , gdb-patches@sourceware.org References: <1480395946-10924-1-git-send-email-tom@tromey.com> <1480395946-10924-3-git-send-email-tom@tromey.com> From: Pedro Alves Message-ID: Date: Fri, 02 Dec 2016 13:05:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1480395946-10924-3-git-send-email-tom@tromey.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-12/txt/msg00082.txt.bz2 > @@ -10535,7 +10526,7 @@ open_dwo_file (const char *file_name, const char *comp_dir) > is a list of paths. */ > > if (*debug_file_directory == '\0') > - return NULL; > + return gdb_bfd_ref_ptr (); This provides a good reason to have an implicit construction from nullptr_t. You had it in the original gdbpy_reference submission, but I had asked to remove it. If we add it back, these cases could be more clearly written as "return NULL/nullptr". Could you do that, and then drop all the hunks like: > - return NULL; > + return gdb_bfd_ref_ptr (); ? > @@ -658,82 +654,70 @@ solib_aix_bfd_open (char *pathname) > } > filename_len = sep - pathname; > > - filename = xstrprintf ("%.*s", filename_len, pathname); > - cleanup = make_cleanup (xfree, filename); > - member_name = xstrprintf ("%.*s", path_len - filename_len - 2, sep + 1); > - make_cleanup (xfree, member_name); > + gdb::unique_xmalloc_ptr filename > + (xstrprintf ("%.*s", filename_len, pathname)); > + gdb::unique_xmalloc_ptr member_name > + (xstrprintf ("%.*s", path_len - filename_len - 2, sep + 1)); I think these could be: std::string filename = string_printf ("%.*s", filename_len, pathname); std::string member_name = string_printf ("%.*s", path_len - filename_len - 2, sep + 1)); > > /* Calling solib_find makes certain that sysroot path is set properly > if program has a dependency on .a archive and sysroot is set via > set sysroot command. */ > - found_pathname = solib_find (filename, &found_file); > + found_pathname = solib_find (filename.get (), &found_file); > if (found_pathname == NULL) > perror_with_name (pathname); > - archive_bfd = solib_bfd_fopen (found_pathname, found_file); > + gdb_bfd_ref_ptr archive_bfd (solib_bfd_fopen (found_pathname, found_file)); > if (archive_bfd == NULL) > { > warning (_("Could not open `%s' as an executable file: %s"), > - filename, bfd_errmsg (bfd_get_error ())); > - do_cleanups (cleanup); > - return NULL; > + filename.get (), bfd_errmsg (bfd_get_error ())); > + return gdb_bfd_ref_ptr (); > } > > - if (bfd_check_format (archive_bfd, bfd_object)) > - { > - do_cleanups (cleanup); > - return archive_bfd; > - } > + if (bfd_check_format (archive_bfd.get (), bfd_object)) > + return archive_bfd; > > - if (! bfd_check_format (archive_bfd, bfd_archive)) > + if (! bfd_check_format (archive_bfd.get (), bfd_archive)) > { > warning (_("\"%s\": not in executable format: %s."), > - filename, bfd_errmsg (bfd_get_error ())); > - gdb_bfd_unref (archive_bfd); > - do_cleanups (cleanup); > - return NULL; > + filename.get (), bfd_errmsg (bfd_get_error ())); > + return gdb_bfd_ref_ptr (); > } > > - object_bfd = gdb_bfd_openr_next_archived_file (archive_bfd, NULL); > + gdb_bfd_ref_ptr object_bfd > + (gdb_bfd_openr_next_archived_file (archive_bfd.get (), NULL)); > while (object_bfd != NULL) > { > - bfd *next; > - > - if (strcmp (member_name, object_bfd->filename) == 0) > + if (strcmp (member_name.get (), object_bfd->filename) == 0) > break; Then here: member_name == object_bfd->filename > > - next = gdb_bfd_openr_next_archived_file (archive_bfd, object_bfd); > - gdb_bfd_unref (object_bfd); > - object_bfd = next; > + object_bfd = gdb_bfd_openr_next_archived_file (archive_bfd.get (), > + object_bfd.get ()); > } > Otherwise LGTM. Thanks, Pedro Alves