Sorry, haven't had time to read the full series yet, but: David Malcolm <dmalc...@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
