Re: [RFA 00/42] Remove globals from buildsym

[Keith Seitz <keiths@redhat.com>]

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