Re: [freesci-develop] Making FreeSCI code C++ friendly

[Christoph Reichenbach]

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