From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 33908 invoked by alias); 5 Jul 2018 18:21:43 -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 33872 invoked by uid 89); 5 Jul 2018 18:21:42 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS autolearn=no version=3.3.2 spammy=shake, HTo:U*tom, attempted, office 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; Thu, 05 Jul 2018 18:21:40 +0000 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8DA5A307CF34; Thu, 5 Jul 2018 18:21:39 +0000 (UTC) Received: from theo.uglyboxes.com (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 4F95E2A175; Thu, 5 Jul 2018 18:21:39 +0000 (UTC) Subject: Re: [RFA 00/42] Remove globals from buildsym To: Tom Tromey , gdb-patches@sourceware.org References: <20180523045851.11660-1-tom@tromey.com> From: Keith Seitz Message-ID: <86ac6592-2df6-c111-b158-19a9e1fd1873@redhat.com> Date: Thu, 05 Jul 2018 18:21:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180523045851.11660-1-tom@tromey.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-07/txt/msg00124.txt.bz2 On 05/22/2018 09:58 PM, Tom Tromey wrote: > I've long wanted to remove the globals from buildsym and generally > clean it up. I've finally tackled this project, and this series is > the result. YAHOO!!! > Also, I simultaneously wrote this series and learned about some the > workings of buildsym. So, there are some cases where something is > done -- say, an assertion added or a variable made static -- only to > be un-done later in the series. Reordering seemed generally painful > so I have left it as is. That's reasonable. > The general idea behind the patches is to move each global variable > (or related set of global variables) into the existing > buildsym_compunit structure. Along the way, some minor cleanups are > done, for example moving stabs-specific things to stabsread. Nice. > Once all of the state is in buildsym_compunit, it is put into > buildsym.h for use by symbol readers. Here, I've converted the DWARF > reader to use the new-style API, leaving the other readers alone. > (The other readers continue to rely on a global, but now only one.) There are other symbol readers besides DWARF? :-P > I haven't tried much to clean up buildsym itself. The API is just as > unwieldy as ever -- it just no longer has global state. I have, > however, replaced some data structures with self-managing ones. > > There are some holes with the series that you may wish to consider. > > * Perhaps some more comments could be added. > > * There are still some stabs-specific hacks in buildsym.c that I have > not attempted to remove. > > * There are some remaining calls to set_last_source_file (NULL) that > could perhaps be removed as unnecessary. I did not check. Upon a "cursory" inspection, the most common nit that I'll mention is moving comments to header files (from corresponding .c files). TBH, our coding standard appears to be in turmoil right now as we shake out C++ (at least it is in my mind!), so feel free to ignore these types of comments. > Regression tested by the buildbot. I also did a reasonable, but not > exhaustive, amount of testing here. I've at least smoke-tested the > stabs reader by running some tests with --target_board=stabs. I've regtested it locally, too, reproducing a stgit with all of the patches. All looks good. In general, I didn't find anything glaringly wrong (but I am just back from a `long' vacation and still a little "out of the office"), but I will respond to individual patches with questions and the like. Thanks, Keith