Re: [PATCH 01/16] read-md.c: Add various cleanups to ~rtx_reader

[Richard Sandiford <rdsandiford@googlemail.com>]

Sorry, haven't had time to read the full series yet, but:

David Malcolm <dmalcolm@redhat.com> writes:
> On Wed, 2016-10-05 at 17:51 +0200, Bernd Schmidt wrote:
>> On 10/05/2016 06:14 PM, David Malcolm wrote:
>> > The selftests for the RTL frontend require supporting multiple
>> > reader instances being alive one after another in-process, so
>> > this lack of cleanup would become a leak.
>> 
>> > +  /* Initialize global data.  */
>> > +  obstack_init (&string_obstack);
>> > +  ptr_locs = htab_create (161, leading_ptr_hash, leading_ptr_eq_p,
>> > 0);
>> > +  obstack_init (&ptr_loc_obstack);
>> > +  joined_conditions = htab_create (161, leading_ptr_hash,
>> > leading_ptr_eq_p, 0);
>> > +  obstack_init (&joined_conditions_obstack);
>> > +  md_constants = htab_create (31, leading_string_hash,
>> > +			      leading_string_eq_p, (htab_del) 0);
>> > +  enum_types = htab_create (31, leading_string_hash,
>> > +			    leading_string_eq_p, (htab_del) 0);
>> > +
>> > +  /* Unlock the stdio streams.  */
>> > +  unlock_std_streams ();
>> 
>> Hmm, but these are global statics. Shouldn't they first be moved to 
>> become class members?
>
> [CCing Richard Sandiford]
>
> I tried to move these into class rtx_reader, but doing so rapidly
> became quite invasive, with many of functions in the gen* tools
> becoming methods.

Is that just to avoid introducing explicit references to the global
rtx_reader object in the gen* tools?  If so, then TBH adding those
references sound better to me than tying generator-specific functions
to the rtx reader (not least because most of them do more than just
read rtl).

> Arguably that would be a good thing, but there are a couple of issues:
>
> (a) some of these functions take "vec" arguments; moving them from
> static functions to being class methods requires that vec.h has been
> included when the relevant class decl is parsed.

I don't think including vec.h more often should be a blocker though. :-)

> (b) rtx_reader turns into a bug dumping ground of methods, for the
> union of all of the various gen* tools.
>
> One way to alleviate these issues would be to do the split of
> rtx_reader into base_rtx_reader vs rtx_reader from patch 9 of the kit:
>   https://gcc.gnu.org/ml/gcc-patches/2016-10/msg00273.html
> and perhaps to split out part of read-md.h into a new read-rtl.h.
>
> Before I reorganize the patches, does this approach sound reasonable?
>
> Alternatively, a less invasive approach is to have an accessor for
> these fields, so that things using them can get at them via the
> rtx_reader_ptr singleton e.g.:
>
> void
> grow_string_obstack (char ch)
> {
>    obstack_1grow (rtx_reader_ptr->get_string_obstack (), ch);
> }
>
> and similar.

I think it's OK for the generators to refer rtx_reader_ptr directly.
Obviously that makes the patches more invasive, but hopefully the
extra changes are mechanical.

Thanks,
Richard