The International Simutrans Forum

Development => Patches & Projects => Incorporated Patches and Solved Bug Reports => Topic started by: isidoro on September 29, 2018, 02:15:47 PM

Title: C++11-only feature in simworld.cc
Post by: isidoro on September 29, 2018, 02:15:47 PM
In simworld.cc:4562         std::string savename = filename;
4563         savename.back() = '_';
4564

method back of basic_string is a C++11 feature and prevent the file from compiling in state-of-the-art Linux with -ansi flag.
Title: Re: C++11-only feature in simworld.cc
Post by: prissi on September 29, 2018, 02:25:32 PM
Ok, how would do this?
savename[savename.length()-1] = '_';
Title: Re: C++11-only feature in simworld.cc
Post by: DrSuperGood on September 29, 2018, 03:00:27 PM
Quotestate-of-the-art Linux with -ansi flag
These two are kind of contradictory. It is "state-of-the-art" but you are running it in a mode targeting a specification from 1998. A Linux build with GCC from 2008 could quite possibly produce the same results.
QuoteOk, how would do this?
savename[savename.length()-1] = '_';
Yes that seems about right.

This still assumes the last byte is always a single byte code point. If it is a multi-byte code point the result will likely be an invalid string.
Title: Re: C++11-only feature in simworld.cc
Post by: Ters on September 29, 2018, 04:37:44 PM
Quote from: DrSuperGood on September 29, 2018, 03:00:27 PMThis still assumes the last byte is always a single byte code point. If it is a multi-byte code point the result will likely be an invalid string.
Since users have no input over the file name extension for save games, that assumption should be solid in this case. There might be similar problems elsewhere in the code, though.
Title: Re: C++11-only feature in simworld.cc
Post by: isidoro on September 29, 2018, 09:48:52 PM
Quote from: prissi on September 29, 2018, 02:25:32 PM
Ok, how would do this?
savename[savename.length()-1] = '_';

Can savename be the empty string?
Title: Re: C++11-only feature in simworld.cc
Post by: DrSuperGood on September 30, 2018, 07:01:56 AM
QuoteCan savename be the empty string?
I am guessing not since apparently it will always at least have the extension at the end of it.

That said for code quality one really should be manipulating strings by glyph position rather than arbitrarily by byte.
Title: Re: C++11-only feature in simworld.cc
Post by: Dwachs on September 30, 2018, 04:34:20 PM
why not

savegame.append( "_" )

? This would ignore all the trouble with multi-byte, empty strings, and what not.
Title: Re: C++11-only feature in simworld.cc
Post by: ACarlotti on September 30, 2018, 07:24:40 PM
It makes the filename longer, which could cause problems. Prissi referred to this issue on the other thread:
Quote from: prissi on September 24, 2018, 02:19:51 PM... and will not lead into troubles should appending .part make a too long filename for that system
Title: Re: C++11-only feature in simworld.cc
Post by: DrSuperGood on October 01, 2018, 09:01:26 AM
If it is a temporary file name, one could generate one from a time stamp. Fixed length so no concern of becoming too long. When the save is moved, one could rename it properly.
Title: Re: C++11-only feature in simworld.cc
Post by: prissi on October 01, 2018, 01:53:49 PM
savename cannot be empty, otherwise fopen woudl fail. An empty filename will result in a file ".sve", so there is for sure at least four bztes in savename.
Title: Re: C++11-only feature in simworld.cc
Post by: Ters on October 01, 2018, 04:30:16 PM
Quote from: DrSuperGood on October 01, 2018, 09:01:26 AM
If it is a temporary file name, one could generate one from a time stamp. Fixed length so no concern of becoming too long. When the save is moved, one could rename it properly.
Well, strictly speaking, any filename longer than five (four?) characters/bytes (including extension) could be too long. And that is assuming the player can save any games at all.

For autosave/load to work, there must be at least room for a 14 byte filename. Since the temp file does not actually need an extension, that would be enough for some kind of timestamp.
Title: Re: C++11-only feature in simworld.cc
Post by: DrSuperGood on October 01, 2018, 07:11:31 PM
QuoteWell, strictly speaking, any filename longer than five (four?) characters/bytes (including extension) could be too long. And that is assuming the player can save any games at all.
What OS that is supported has those limits? Windows the path length is 255 characters odd, extendable to over 6,000 odd if a better file system API was used.

In any case on Windows an appropriate temporary folder could be used which should have more free characters available. If not then chances are other applications would be breaking as well.
Title: Re: C++11-only feature in simworld.cc
Post by: Ters on October 01, 2018, 07:43:56 PM
Quote from: DrSuperGood on October 01, 2018, 07:11:31 PM
What OS that is supported has those limits? Windows the path length is 255 characters odd, extendable to over 6,000 odd if a better file system API was used.

That is for the entire path, not just the file name. So the OS I had in mind was Windows, although I guess any OS can give these kinds of problems if the user is insane enough about naming directories.
Title: Re: C++11-only feature in simworld.cc
Post by: isidoro on October 01, 2018, 10:36:20 PM
Why not let the task to the OS?  At least for Linux, you have mkstemp.  Don't know (and don't want to know  ;D )about other OSes.
Title: Re: C++11-only feature in simworld.cc
Post by: DrSuperGood on October 01, 2018, 11:31:22 PM
QuoteWhy not let the task to the OS?  At least for Linux, you have mkstemp.  Don't know (and don't want to know   )about other OSes.
Because the file might need to persist. From what I understand temporary file IO is intended for temporary storage locations for bulk data and is largely a legacy left from pre virtual memory days where one could not abuse a memory mapped page file for extra storage space in an application with potentially terabytes of virtual memory.

What is happening here is that a file is temporarily written to another location before being moved to its intended persistent location. This is a safety in case Simutrans crashes during save so that existing saves only get overwritten with complete saves. OSes might implement file moves more efficiently than a delete/truncate then write since at a low level they may not need to move the file data around, only the file system entries to the file data.
Title: Re: C++11-only feature in simworld.cc
Post by: Ters on October 02, 2018, 05:52:14 AM
Quote from: isidoro on October 01, 2018, 10:36:20 PM
Why not let the task to the OS?  At least for Linux, you have mkstemp.  Don't know (and don't want to know  ;D )about other OSes.

In this case, we want to create the file in the same directory, or at least on the same volume. mkstemp gives no control of this. Saving temporarily in the save directory has two major advantages. Firstly, as DrSuperGood also points out, one does not need to move the data from one volume to another, which takes time (and increases risk of failure). The OS only needs to update the directory entries. (It could create links across volumes, but this would be confusing. If the save game directory is on a thumb drive, you want the data to be on the thumb drive.) Secondly, the temp file can be found just as easily by the user if the move operation fails. The move operation may then be completed manually. The user may also send an incomplete save when reporting a bug in the saving.

The only drawback is that the volume needs to have room for two save games, even if only temporarily: the one being written, and the previous one. However, I haven't had such problems with insufficient storage space since the days of (not so) floppy disks.