Hello
On Wed, 2010-08-25 at 06:50 -0700, Ian Lance Taylor wrote: > Basile Starynkevitch <bas...@starynkevitch.net> writes: > > > On Tue, 2010-08-24 at 15:19 -0700, Ian Lance Taylor wrote: > >> > >> > However, our patch also added some improvements to gengtype itself > >> > >> Improvements are desirable, but if it is all possible you should > >> separate these improvements from your other work. It is very hard to > >> review patches which combine different unrelated ideas in a single diff. > > > > > > We only improved some minor points of gengtype when we had bugs related > > to code that we found difficult to understand or to read. In particular > > > > * replacement of the kludge of putting a language bitmask four bytes > > before the path name of input files with a real structure for input > > files. > > > > * replacement of the get_output_file_with_visibility code with a > > sequence of regular-expression rules. > > > > None of these patch is unrelated to our main work of adding persistency > > to gengtype. We had to make these improvements to make the whole thing > > work. > > That is fine, but I am going to reiterate: from your description, those > changes can be separated from the rest of the patches. Therefore, they > should be sent in separately. I am not at all sure we would be able to sent them separately, in such a way that gengtype still works correctly with each patch applied one by one. My current thinking is that we probably will be able to sent two patches. first, some cleanup of the gengtype code, and second the persistent state machinery (which should go in a new gengtype-state.c file). I am not sure we are able (in a reasonable time) to make more fine grain patches. The cleanup patch is perhaps the less straightforward to review, but it is much smaller than the persistent state machinery (which is just a large set of parsing & printing routines of gengtype main data types...). The current state of the patch is on http://starynkevitch.net/Basile/gengtype-r163550-25august2010.diff and it is still not yet working entirely, nor is it ready for submission (and as Laurynas kindly noticed we also have some indentation issues). However, Jeremie & me did some significant progress today. Our new gengtype works correctly for [generating all GTY related files in] cc1 (but not yet for cc1plus...). Today's bug is that #define ggc_alloc_cleared_named_label_entry is not generated but it should (apparently the USED_BY_TYPED_GC_P test in write_typed_alloc_defns becomes false but it should not). I suspect it is related to lang_tree_node, but I don't have a clear understanding of lang related stuff. > > In order to get a patch accepted, your goal should always be to provide > minimal isolated changes that are easy to understand. I am not sure to be able to provide a proven minimal set of changes. I have no reasonable way of proving or convincing that the set of changes (probably two consecutive patches) is minimal. And our previous gengtype patch (only adding program arguments to prepare the persistent state work) was very small but have not being reviewed till acceptance. So I am not sure that small patches are enough! Should we try to improve GTY documentation? Apparently ptr_alias is not documented in gty.texi. Besides, testing any gengtype change is very expensive for us (& probably for others), because we have to regenerate all the gt*.[ch] generated files & rebuild the entire compiler at each test. Even with the help of ccache, that takes a lot of time. > The larger the > patch, the harder it is to understand and review. Always make changes > as small as possible. Separate cleanups from substantive changes. A question for gengtype experts: what exactly determines the set of generated gt*.[ch] files? We have the fuzzy impression that even some ada specific files are generated while ada was not configured but we may be wrong. Laurynas, Ian, others, could you please explain in a few words how you believe the various languages (cp, ada, objc, ...) work inside gengtype? Obviously, the bitmask is representing a set of languages, but neither Jérémie nor me Basile grasped all the details... I am not sure to understand what exact parts of gengtype is using the [cp] & [ada] ... tags in gtype-input.list, and I am not sure to grasp what TYPE_LANG_STRUCT & TYPE_PARAM_STRUCT are exactly for, even if I do guess a bit. Does the set of configure-d languages matter to gengtype? If yes, how? I tend to imagine that our latest bug is related to the above questions. Thanks for reading. Regards. -- Basile STARYNKEVITCH http://starynkevitch.net/Basile/ email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359 8, rue de la Faiencerie, 92340 Bourg La Reine, France *** opinions {are only mine, sont seulement les miennes} ***
