Jordi, sorry for the late reply! In any case, thank you for your hard work and for the patch! I had a brief look at it, and I have some comments below:
> Would it help to make several split patches? With what criterion? One > for the casts, one for the reserved keywords, other for more specific > stuff...? While I'm all in favour of small, split patches, there's a more significant way in which you can make yours more digestible, namely by eliminating everything cfsml. The cfsml crud is only used by pre-0.6 (the ``stable'' series); there, we used it to implement savegames. We don't have savegames in the 0.6 (``glutton'') series right now, and are not using the cfsml-generated code at the moment. While there's nothing wrong with the changes you made to cfsml.pl (yes, it's Perl, I repent), they don't affect the program ATM. It is not clear whether cfsml will ever again be used to implement savegames in glutton. Apart from that, here is my opinion on the patch: 1. Even without cfsml it will be big. For processing, I'd suggest splitting it up (anyone could do that, of course, wouldn't have to be you) to be able to split up the work of merging it in, if we are to accept it. 2. I noticed a few places here and there where you might have been able to avoid typecasts altering local variable types. These are small things, and with sufficiently small patches, they should be easy to revisit. 3. I don't like the renaming of variables such as ``destroy'' and ``class'' and ``new'' much. Now I do realise that those changes are neccessary to convert the core of FreeSCI to be compilable with a present-day C++ compiler, but, as I mentioned before, I'm doubtful as to whether that is a helpful change in the first place. I'd suggest that you split the patch between ``cast fixes'' and ``C++ keyword renaming''. Personally, I would then suggest accepting the ``cast fixes'' patch with minor revisions (which we could do while merging, cf point 2), and to reject the ``C++ keyword renaming'' part of the patch, unless someone can point me to a good reason for applying it (being able to compile with just one compiler is not much of an argument; recall that C is still more widely available than C++, though both should be readily avialable on all of our target platforms). If there is some motion to rewrite parts of the inner FreeSCI guts in C++, we can pull out the patch again, but for the time being I see it as slightly confusing and not useful. In any case, thank you very much for the effort you put into this! The vast majority of what you did should be useful even just from the C side of things. -- Christoph _______________________________________________ FreeSCI-develop mailing list [email protected] http://lists.nongnu.org/mailman/listinfo/freesci-develop
