Skip to main content ↑ ↓
Topic: Source code bugs in simutrans-experimental 7.2, & patch (Read 4858 times) previous topic - next topic

Source code bugs in simutrans-experimental 7.2, & patch

This is based on downloading the 'master' branch.

1 - It doesn't work unless SLIST_FREIGHT is defined, which isn't on by default.

I fixed the simplest of these in the attached patch.  The more complicated ones require serious rewriting of the
iteration code.  Specifically in simconvoi.cc at lines 2926 and 3946.  You'll have to write the vector_tpl versions of these.

Would you prefer to totally disable the vector_tpl implementation for freight?  I could give you a patch to do that.

Alternatively, change the default FLAGS in config.template:
FLAGS =  -DSTEPS16 -DSLIST_FREIGHT

This *IS* included in my patch.

You'll probably want to fix or disable the vector_tpl implementation later anyway, but this would avoid the *repeated* failures to compile automatically under Linux.

2 - There are some other minor errors with GCC which I fixed in the attached patch.   MAXUINT32 is an undefined quantity, so I included <limits.h> to get UINT_MAX.  <string.h> was needed for strlen in a file.

3 - There are many warnings with GCC, some of which I fixed so I could better spot the errors.  This is mostly unnecessary 'consts' on value returns, and some signed-unsigned comparisons, which I fixed by adding appropriate-seeming casts (though I didn't get all of those).  There was also a unclear expression due to missing parentheses.  I rearranged some case statements to add default: clauses in order to eliminate a huge number of warnings.   A type needed to be "GCC_PACKED" in order for the type it was contained in to be "GCC_PACKED".   A sprintf required a length prefix, and another one had excess arguments. 

4 - There are several cases of casting away constness, which I did *not* fix.  These *should* be fixed as they can create very hard to find bugs.

5 - There are several initialization-order warnings, which are well beyond my ability to debug.

Patch is attached; with this it compiles with GCC.  Feel free to apply it as multiple separate patches in git; each file's edits are pretty much independent.  Please do apply it though!

I'll try to stomp warnings in standard some other time...

Re: Source code bugs in simutrans-experimental 7.2, & patch

Reply #1 –
Neroden,

thank you very much indeed for your work - it is much appreciated. I have applied your patch (manually), and pushed the fixed code to the master branch, so hopefully, the Linux builds should be available to-morrow. Your work is much appreciated.

As to removing the vector implementation of freight - that would indeed be a worthwhile thing to do, as the vector version is now obsolete (it was put in at a time when the slist_tpl had bugs in it) If you are able to produce a patch for that, that would be most appreciated.

As to casting away constness - I did not think that I'd done that. Where in the code is that apparent? I always try to avoid that, and thought that I'd succeeded in this case. As for the initialisation order warnings - are you sure that those come from code specific to Simutrans-Experimental?

Thank you again very much indeed for your work on this.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Re: Source code bugs in simutrans-experimental 7.2, & patch

Reply #2 –
Quote from: jamespetts – on
Neroden,

thank you very much indeed for your work - it is much appreciated.
OK, I'm starting to get embarrased by the profusion of your thanks.   :-[  This sort of code cleanup and debugging is what I do to relax, so I'm happy to help.

Quote
I have applied your patch (manually), and pushed the fixed code to the master branch, so hopefully, the Linux builds should be available to-morrow. Your work is much appreciated.

As to removing the vector implementation of freight - that would indeed be a worthwhile thing to do, as the vector version is now obsolete (it was put in at a time when the slist_tpl had bugs in it) If you are able to produce a patch for that, that would be most appreciated.

OK!  Should I prepare a patch for *standard* to get rid of it too?  

I managed to finish my taxes early (due in the US on April 15th) so I may have some time....

Quote
As to casting away constness - I did not think that I'd done that. Where in the code is that apparent?   I always try to avoid that, and thought that I'd succeeded in this case.
Those are pretty subtle.  I already solved some of them in the patch I attached; I believe they all involve accessors (get_*) with const returns which are used by mutators within cl**** setup code to get variable references, which are then modified (during cl**** setup/loading/initialization).  The references are cast away from const for this purpose.  Some of the casts may also be in standard.  

C++ reference variables can be a lot trickier to use than pointers.  In one case I rewrote the mutator to access the variable directly (requiring conversion of the variable from 'private' to 'protected' as the mutator is in a descendant cl****).  The others are less trivial -- in at least one case I believe it requires creating a second accessor, because the mutator isn't in the same cl**** as the variable.

I'll try to come up with separate patches for the remaining const-cast warnings.  When I get a chance.

Quote
As for the initialisation order warnings - are you sure that those come from code specific to Simutrans-Experimental?
Actually, I'm dead sure that code is in standard.  And the warnings are unimportant, as the code is initialization-order-proof.  It just clutters up the error/warning log.

Re: Source code bugs in simutrans-experimental 7.2, & patch

Reply #3 –
Neroden,

apologies for overthanking you - I always appreciate people ****isting with the code, however! As to patches for Standard: I have no say on whether patches for Standard are included. However, there is no vector implementation of freight in Standard - the vector implementation of freight was first introduced in Experimental because of the bugs in slist_tpl. It is now no longer needed, so can be excised entirely.

As to casting away constness, if you would like to produce a patch for that (which may well be better off as patch for Standard, since I think that most of these issues are in Standard; I tend to use pointers rather than references when I write new code), then feel free to do so, but it is Prissi and Dwachs who decide whether they go in, rather than me.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Re: Source code bugs in simutrans-experimental 7.2, & patch

Reply #4 –
The casting away from standard is done in only two places to avoid exposure of an item of a different cl**** which is accessed only once from another cl****. Any attempts to remove this has produced much more warnings and likely causes for failures in other places.

Also standard compiles with very little warnings, as usually before a release all those are tried to be removed. Some of the signed/unsigned stuff cannot be completely removed, since those depend on the definition of char for some functions. Or they were left in, since at those places the code could break with other compilers.

Re: Source code bugs in simutrans-experimental 7.2, & patch

Reply #5 –
Quote from: prissi – on
The casting away from standard is done in only two places to avoid exposure of an item of a different cl**** which is accessed only once from another cl****. Any attempts to remove this has produced much more warnings and likely causes for failures in other places.
One of the two casts only happens in the case of buggy pak addons (ones which override stuff in the base pak).  I don't think it's worth reconstructing the code structure to fix that one, though I can probably come up with a clean way to do it if I spend a month thinking about it.  :-)

I wonder what the other one is?  (Goes to compile.)

Re: Source code bugs in simutrans-experimental 7.2, & patch

Reply #6 –
accessing climate informations in the world dialoge.
 

Re: Source code bugs in simutrans-experimental 7.2, & patch

Reply #7 –
Quote from: prissi – on
accessing climate informations in the world dialoge.

Thanks.  I *will* try to fix that one sometime.  That one *could* cause problems and I *have* had mysterious crashes when I've changed the climate information in the world dialogue.

The solution I'm thinking of is a bit heavyweight; it involves two climate objects, a "const" one and a modifiable one, with the "const" one being initialized from the modifiable one at world creation time.