[PATCH 01/16] read-md.c: Add various cleanups to ~rtx_reader
Richard Sandiford
rdsandiford@googlemail.com
Wed Oct 12 21:57:00 GMT 2016
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
More information about the Gcc-patches
mailing list