News:

Simutrans Chat Room
Where cool people of Simutrans can meet up.

Patch to start migrating code to C++11

Started by dodomorandi, November 01, 2018, 11:24:40 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

dodomorandi

I started working on modernizing simutrans. With this patch I added the '-std=c++11' flag and fixed all the warnings that both gcc and clang produced when using "-Wall -Wextra -Wpedantic" (therefore the implicit conversions are still there), except for one thing I would like to discuss below. As you can see from the file, most of the changes just involve explicit 'override', but there are some changes related to using raw memory copy/move operations on non trivial classes, some buffers that were a bit too small (only from a theoretical point of view, probably they never caused any problem at all) and some encoding errors (some characters inside literal strings were not encoded in utf8, I was unable to notice it as well). I also removed VLAs (alloca is still there and GCC now use it as well. Touching alloca is something that I can think for the future, not now) and changed some minor things.

The think I want to discuss is the reason why some types, in particular koord3d, are packed. I got a warning for this one because it is possible to access elements of koord3d that are unaligned, and this could reduce performances a bit on x86_64 (honestly, I totally don't know how other architectures handle unaligned types). If the main problem is too much padding, maybe in the future it could be nice to have a SoA to handle multiple koord3d (and the other structs as well if necessary). I am not considering this a problem, I am just writing this to better understand the reasons and have something I can read in some months ;)

Nevertheless, let me know what do you think about the patch.

EDIT: as suggested by many devs, the huge patch will be splitted in many minor patches. The first one has 'override's all over the places. It is worth noting that the only 'DELETED' usage was in "utils/plainstring.h". Other patches will come after.

EDIT: new patch, related to fixing possible buffer overflows. Most of these are almost theoretical (they have been detected by the compiler based on the input data), but I think it is better to avoid potential issues.

TurfIt

'hiding' real changes within a huge patch that mostly changes OVERRIDE to override makes it too easy to miss things...
Source files are ASCII, not UTF8. Corrupting peoples names with gibberish is.... and I believe one of the game strings changed use the character for a symbol.
As far as I know the code in the squirrel/ directories are straight from that project. If changes are needed there shouldn't they go upstream? Makes it more difficult to update if now running a 'patched' squirrel.

All structures are designed/packed around 32 bit. At some point consideration should be given to redoing them all for proper packing in 64 (and abandon 32 bit builds entirely), but for now....  The slowdown in 32 from unpacking is greater than the gain in 64, and 64 gets a perf boost already from the expanded instruction set/registers.
In a long worked on patch by me to expand the multithreading to the simulation routines, I experimented with changing around the memory layout of the objects quite a bit. 'fixing' the horrid default allocations gives more of a speedup than the multithreading! (40% faster for stadtauto_t objects). ofc at my current productivity, another 10 years before this is ready.

There was talk at one point about sticking something else into the wasted space in the koord3ds. Can't remember what. Unaligned access is less of a performance hit than the extra memory accesses to read in the padding. Memory latency sucks, cache space is precious - pack it full of useful stuff.

---
Not directly about moving to C++11, but I do have a patch I've been sitting on to move to std::min/max and get rid of Simutrans own dangerous sint only implementation. I'll try to get that redone in the next week or so.

prissi

#2
Coordinates are packed, because they are used in every object and thus every byte counts. Pakec coors in another structure just take up 5 bytes. You can uses the size switch to check the size of the relevant structures. You have to remember that L1 cache is very small, and the maps are huge. 4096x4096 is 16 Million tiles, and about 48 million trees, not to mention other objects.

Generally, optimising for compiler warnings of a specific compiler and revision was never a thing for SImutrans. MSVC gets an entirely different set of warnings, as did the Intel compiler. So yes, where it makes sense, we can remove.

But for instance the extra trailing semicolons: They are not forbidden (as far as I know) and personally, I think the help humans to realise that things are finished after that line. C++ is rather inconsequential since it require a semicolon after a function declaration be none after a function declaration with body.

OVERRIDE was as a macro because otherwise compilation failed on Haiku and Amiga PC. So I would like to keep it uppercase.

The patch contains file without changes???

And this do not look right, you removed the const to assign it to a const???

- const waytype_t way0 = (const waytype_t)min(desc->get_waytype(0), desc->get_waytype(1));
- const waytype_t way1 = (const waytype_t)max(desc->get_waytype(0), desc->get_waytype(1));
+ const waytype_t way0 = (waytype_t)min(desc->get_waytype(0), desc->get_waytype(1));
+ const waytype_t way1 = (waytype_t)max(desc->get_waytype(0), desc->get_waytype(1));


what is the purpose of changing "char name[]" to "char *name"? The intention of the first is much clearer.

ALso I would remove the comment from all the overide functions, which is useless and one should refer to the original in this case. Honestly, adding overide was not exactly what I was expecting, because it is not required by C++11 ... Especially changing outcommented functions like
-// void report_vehicle_problem(convoihandle_t cnv,const koord3d position) OVERRIDE;
+// void report_vehicle_problem(convoihandle_t cnv,const koord3d position) override;


In simwin id is unsigned, but the routine used signed there. SInce this would be a different routine to read unsigned, it was left as it is.

This I simply oppose. I use C++ for brevity, not for such monsters.

- char objname[256] = { type, type>>8, type>>16, 0 };
+ char objname[256] = {
+ static_cast<char>(type),
+ static_cast<char>(type>>8),
+ static_cast<char>(type>>16),
+ 0
+ };


Changing lengthof(tree_id_to_image) size sizeof() is a bad idea, since its element size might change in future. So we tend to use sizeof for all such situations.

What is the gain of changing if(dx*dy)  to if(dx*d>0)? The thing to test is anyway if(  dx!=0  &&  dy!=0  ) ...

Changing memcopy to std::copy is on a piece of memory is also not really what I though. std::function make sense with std objects. Otherwise memcopy to indicate I am working on memory makes sense to me. All in between cause more confusion, imho.

Anyway, first thanks for going through this. I will change lowercase override to uppercase OVERRIDE, remove the squirrel stuff (since this is verbatim from the squirrel project) and check the rest. Please be patient.

Ters

I also dislike lots of different unrelated changes in one patch/commit.

The change in xref_desc_t from char name[] to char *name is just plain wrong. The former is an array of an undefined length embedded into the structure itself, the latter is pointer. Embedded variable length arrays is a somewhat dirty trick, but skipping a level of indirection and increasing memory locality may be performance critical.

Quote from: prissi on November 02, 2018, 12:58:35 AMOVERRIDE was as a macro because otherwise compilation failed on Haiku and Amiga PC. So I would like to keep it uppercase.
Well, then we can't use C++11! It's one or the other.

dodomorandi

Quote from: dodomorandi on November 01, 2018, 11:24:40 PM'hiding' real changes within a huge patch that mostly changes OVERRIDE to override makes it too easy to miss things...
I did not mean to hide changes. If you prefer, I can split the patch in two separated files.

Quote from: dodomorandi on November 01, 2018, 11:24:40 PMSource files are ASCII, not UTF8. Corrupting peoples names with gibberish is....
Before accusing me of corrupting people names, please check. This is the hex dump of a piece of scrolltext.h before the patch:

00000240  22 20 41 6e 64 72 65 61  73 20 52 f6 76 65 72 22  |" Andreas R.ver"|

After

22 20 41 6e 64 72 65 61  73 20 52 c3 b6 76 65 72  |" Andreas R..ver|

Character ö is, unfortunately, in the "extended" range of the ASCII table. This means that uchar should be used instead of char, but strings have to use chars. Moreover, clang tells you this:
gui/../scrolltext.h:28:12: warning: illegal character encoding in string literal [-Winvalid-source-encoding]
If you try to use that change you will notice that the banner is perfectly fine (however go at the end of the post, there is a note). If you really think that this is wrong, I will revert the change back, but remember that I made this for a reason.

Quote from: dodomorandi on November 01, 2018, 11:24:40 PMIf changes are needed there shouldn't they go upstream?
I did not know what to do about the squirrel code, mainly because I didn't know the status in simutrans respect to the main releases. I generally prefer the changes to go upstream as well, I can see if it is doable.

Quote from: dodomorandi on November 01, 2018, 11:24:40 PMAll structures are designed/packed ...
Ok, I just wanted to understand the reasons. As I said, solutions different than packing require some efforts, I can think about that in the future ;)

Quote from: prissi on November 02, 2018, 12:58:35 AMBut for instance the extra trailing semicolons: They are not forbidden (as far as I know) and personally, I think the help humans to realise that things are finished after that line. C++ is rather inconsequential since it require a semicolon after a function declaration be none after a function declaration with body.
Honestly, I have almost the same opinion. In this case my point was being consistent with "no warning policy", but I can revert back the semicolons for macros. About the end of the namespaces, I honestly really prefer a "//namespace whatever" comment as I did for the longer one. I can do that for the shorter namespaces as well.

Quote from: TurfIt on November 02, 2018, 12:54:29 AMOVERRIDE was as a macro because otherwise compilation failed on Haiku and Amiga PC. So I would like to keep it uppercase.
Reading around, it looks like it is possible to compile for both of them with C++11 compliant compilers (I am not sure about hybrid haiku). Said that, as also Ters said, if we intend to support very old compilers or non-compliant custom builds (like the hybrid GCC2.95/GCC7 of the 32bit haiku, which might work as well because it has GCC7 capabilities), then the idea of modernization does not make sense at all.

Quote from: prissi on November 02, 2018, 12:58:35 AMAnd this do not look right, you removed the const to assign it to a const???
That cast is simply ignored by the compiler. https://godbolt.org/z/obRInJ

Quote from: TurfIt on November 02, 2018, 12:54:29 AMwhat is the purpose of changing "char name[]" to "char *name"? The intention of the first is much clearer.
This is not C++ compliant. It is a C99 extension, and it is not in the C++ standard. https://godbolt.org/z/gLExYG. Nevertheless, it is something that I intended to change in the future. If you want, we can keep the array version, just remember that it should be changed.

Quote from: TurfIt on November 02, 2018, 12:54:29 AMALso I would remove the comment from all the overide functions, which is useless and one should refer to the original in this case. Honestly, adding overide was not exactly what I was expecting, because it is not required by C++11 ... Especially changing outcommented functions like
About the comments, you are totally right. I replaced the macro and I missed that some of them were on comments. The override keyword is pretty important in terms of virtualization: if you forget to "override" a virtual function you get a warning. This is redundant on functions marked as "virtual", but I found many of those that were not virtual. When calling these functions on a base class, the derived one was not called. 'override' helps in order to express intents.

Quote from: TurfIt on November 02, 2018, 12:54:29 AM
In simwin id is unsigned, but the routine used signed there. SInce this would be a different routine to read unsigned, it was left as it is.
I did this because I noticed that magic_none is -1, I thought that the type could not be unsigned. I will revert it back.

Quote from: prissi on November 02, 2018, 12:58:35 AMThis I simply oppose. I use C++ for brevity, not for such monsters.
We see it in a different way. These are narrowing conversions, and I consider explicitness a good thing in these cases. I can revert it back, but have a different opinion.

Quote from: prissi on November 02, 2018, 12:58:35 AMChanging lengthof(tree_id_to_image) size sizeof() is a bad idea, since its element size might change in future.
The problem here is that the whole size of 25600 was not written using "lengthof", because tree_id_to_image is a 2d array.

Quote from: prissi on November 02, 2018, 12:58:35 AMWhat is the gain of changing if(dx*dy)  to if(dx*d>0)? The thing to test is anyway if(  dx!=0  &&  dy!=0  ) ...
The gain is readability. But you are right, "dx!=0 && dy!=0" is better, I will change that.

Quote from: prissi on November 02, 2018, 12:58:35 AMChanging memcopy to std::copy is on a piece of memory is also not really what I though. std::function make sense with std objects. Otherwise memcopy to indicate I am working on memory makes sense to me. All in between cause more confusion, imho.
I only put a couple of std::copy and std::move for reasons: it does exactly the same thing for trivially constructible and assignable types, but it also work for other types. Almost everything we touch is "memory", but this does not mean you don't have to use abstractions.

Quote from: prissi on November 02, 2018, 12:58:35 AMAnyway, first thanks for going through this. I will change lowercase override to uppercase OVERRIDE, remove the squirrel stuff (since this is verbatim from the squirrel project) and check the rest. Please be patient.
Don't take me wrong for my words: I really appreciate your work, and I am happy to do this. I think that criticisms are necessary to do things better, and I am happy with this ;).

Quote from: Ters on November 02, 2018, 06:43:51 AMI also dislike lots of different unrelated changes in one patch/commit.
As I said before, you are probably right. I will split the current patch in patches, and I will revert some things when necessary.

Quote from: Ters on November 02, 2018, 06:43:51 AMbut skipping a level of indirection and increasing memory locality may be performance critical.
As I said, this is C99 and not C++. That variable just decays into a pointer. The fact that later an 'alloca' is used for that variable is unrelated (in fact alloca returns a pointer).

Quote from: Ters on November 02, 2018, 06:43:51 AMWell, then we can't use C++11! It's one or the other.
Totally agree. What do you want to do?

NOTE: something is not working. I mainly wanted an initial feedback, and I am happy that now I got it. Nevertheless, I need to fix the problem, it looks like something gone wrong with one (or more) virtual calls. I will write you about this as soon as I look at it.

prissi

Last time I checked Haiku it had GCC 4.5.x, so it supports almost all C++11 but override.

I am also a little puzzled, why gcc wants that the derived classes to specify override again. If the base class has it, the derived classes will not need it, since they fulfilled the overriding already (and are anyway virtual). So why put the compiler insist to put it it into classes that are not further bases for classes? (No problem adding it now, but I am not so sure I remember them next time again ... )

The scrolltest issue is tricky. Simutrans uses the english translation at the start until recently this means Latin code page. Now almost all are UTF-8, so it makes sense to encode it as that. But the source files should be ASCII for char and encode these chars as hex, since this is the only fully portable way.

dodomorandi

Quote from: prissi on November 02, 2018, 12:21:45 PMLast time I checked Haiku it had GCC 4.5.x, so it supports almost all C++11 but override.
The non-32bit version looks like to have been ported to GCC7 (and a GCC8 port is on the way AFAIK). The 32bit... dunno what it can support, being a hybrid GCC2/7  ???. I can try to make some tests before considering the patch portable.

Quote from: prissi on November 02, 2018, 12:21:45 PMI am also a little puzzled, why gcc wants that the derived classes to specify override again. If the base class has it, the derived classes will not need it, since they fulfilled the overriding already (and are anyway virtual). So why put the compiler insist to put it it into classes that are not further bases for classes? (No problem adding it now, but I am not so sure I remember them next time again ... )
AFAIK 'override' has been introduced to avoid a very bad situation: if you have a slightly different signature between the base and the derive class, you don't get a virtual function and you won't notice. If you use 'override' instead of 'virtual', the function must override the base class. It makes life easier, catching errors at compile-time. (the reason I did not expect a problem is exactly that I didn't expect to change any behaviour, but I will debug it ;))

Quote from: prissi on November 02, 2018, 12:21:45 PMThe scrolltest issue is tricky. Simutrans uses the english translation at the start until recently this means Latin code page. Now almost all are UTF-8, so it makes sense to encode it as that. But the source files should be ASCII for char and encode these chars as hex, since this is the only fully portable way.
I understand your point. Probably this thing will deserve a completely separated patch. I did not expect to cause troubles because it is a pure standard C++ string, but on the other hand I understand that it can cause strange behaviours in edge cases. I will revert that.

wlindley

Indirectly related, but should we start another thread about modernization of file structure, including replacing "pak" format with a zipped collection of the text pakset configuration and .png graphic files?  In this day and age we should use plain formats rather than weird packed and incompatible-with-itself binary formats that need to change every time our internal structures are modified.

Yona-TYT

I should also move this to the larger projects, I think this goes for a while.  ;)

Ters

Quote from: dodomorandi on November 02, 2018, 10:02:14 AMAs I said, this is C99 and not C++. That variable just decays into a pointer.
It decays when assigned from. The storage type is still a character array. At least that is the way GCC 7.3.0 implements it.

This prints very different values for a:

#include <cstdio>
#include <cstdlib>
#include <cstring>

struct Foo {
   int i;
   char a[];
};

struct Bar {
   int i;
   char *a;
};

int main() {
   Foo *foo = (Foo *) malloc(sizeof(Foo) + 4);
   memset(foo, 0, sizeof(Foo) + 4);
   printf("%p %p\n", foo, foo->a);
   
   Bar *bar = (Bar *) malloc(sizeof(Bar) + 4);
   memset(bar, 0, sizeof(Bar) + 4);
   printf("%p %p\n", bar, bar->a);
   
   return 0;
}

The first line writes two addresses four bytes apart. In the second line, the second address is zero. (The code is not pure C++, as this was quicker to write in a hurry. That I use malloc doesn't change how struct members are accessed later on.)

Furthermore, if C++ did this differently from C, then the Win32 API would be unusable from C++. That API is full of arrays tacked onto the end of structs.

Quote from: dodomorandi on November 02, 2018, 10:02:14 AMThe fact that later an 'alloca' is used for that variable is unrelated (in fact alloca returns a pointer).
I don't understand what you are talking about. The entire structure is allocated using new(node.size - 4 - 1) xref_desc_t(). name is never directly allocated.

dodomorandi

Thank you Ters, and sorry. I was totally wrong about the behaviour of the compiler, and what you are saying is definitely right about it. Indeed, that was the reason of the bug I introduced.

Quote from: Ters on November 02, 2018, 05:59:18 PMI don't understand what you are talking about.
Forget it, when I checked it after posting, I realized it was allocated by new.

About Haiku: I installed the 32bit version in a VM, and I understood what I was reading about hybrid GCC: it has both 2.95 and 7.x version of if. Obviously, the GCC7 version has full support of C++11 (and more), and the 2.95 not. Therefore, I don't know if we can consider C++11 available on Haiku 32bit or not  :o

dodomorandi

#11
I edited the original post, and I included the patch only for the overrides.
EDIT I also added a further patch for buffers size.

Dwachs

Comitted the buffer-length patch. Thanks!

How do you came up with these numbers?
Parsley, sage, rosemary, and maggikraut.

dodomorandi

I don't remember if it thanks to clang or gcc, but one of the two gave me warnings about possible buffer overflow, and the needed size. I would not have noticed without the help of the compiler  ;D

DrSuperGood

A lot of those buffers should probably be removed and replaced with dynamic string related API calls, or at least use size constants explaining why such buffer sizes were chosen. For example why 1023 characters and not 1024?

dodomorandi

I totally agree. I wanted to start with trivial changes, I am already trying to something more complex. There are some minor changes that were present in the big patch, I will post them in small changes during these days.

prissi

#16
About the buffer for file operations: There is a define PATH_MAX, which gives the maximum length of characters. That is usually 1024 for windows and 4096 on linux. Maybe we should use such a define for most paths/filenames.

EDIT: submit as 8626

dodomorandi

Just for curiosity: is there any particular reason of not using boost for heavyweight features like filesystem support? I am generally worried about problems with old versions, but for specific things the support is stable since old versions. Just asking, nothing more ;)

Ters

Boost is a behemoth that takes ages to build. I prefer that Simutrans has as few dependencies as possible.

I'd also like to say that if it is not at all obvious why an array needs to have a specific size, it is not a good idea to have a plain array. At the very least, something explaining the hardcoded size is in order.

dodomorandi

I really need your feedback about one thing that I want to code. It requires a bit of work, and I would like to hear your opinions to avoid wasting some efforts.

I wanted to get rid of the C99 variable length syntax. I already wrote a stub and I know how to do it, but to do this correctly I need to change a bit the memory management of obj_reader_t::load. In practice, I want to get rid of manual allocations using new, replacing them with RAII approaches like std::unique_ptr. Obviously I am quite new with simutrans codebase, and it took me some attempts to find a valid and coherent approach, but I should be there.

Now, my point is that I wanted to perform the memory management change patch, and then the other one for VLAs. Unfortunately, it looks like that GCC stopped allowing me to use C99 VLAs after some C++11 changes, therefore I need to write a single bigger patch :(. I would like your feedback because it is a bit of work, and maybe there are different possible approaches for the problem. I will show my idea, so you can give me your feedbacks.

I want to write a templated class that is responsible for RAII allocation and construction, something like std::unique_ptr. The main difference is that this will allow to have "tail" variable length array of data. Let's call the class "Vla", the idea was something like the following:

template</* gimme a sec about this */ T, typename Tail, typename TailSizeT = std::size_t>
class Vla {
    template <typename... Args>
    explicit Vla(TailSizeT tail_size, Args && ... args);
};


As I said, Vla will be responsible of allocating our type... or a sort of. In practice, we need to access the data in the "tail", and I wanted to avoid useless boilerplate, and at the same time I want zero overhead. Therefore, my idea is to create a helper class to allow easy managing of the VLA-related data, and to use CRTP to have static inheritance. In the previous example, T would be a template <typename> class T

The following example struct

struct legacy_data_t {
    double d;
    int i;
    unsigned short data_size;
    char data[];
};

would become something like

template <typename V>
struct new_data_base_t {
    double d;
    int i;
};

using new_data_t = Vla<new_data_base_t, char, unsigned short>;

You can imagine that Vla will have an API very similar to std::unique_ptr, but instead of returning a pointer to a new_data_base_t (it can't, it is a templated class!  :P), it will give back a pointer to a VlaWrapper, something like

template <template <typename> class T, typename Tail, typename SizeTailT>
struct VlaWrapper final : T<VlaWrapper<T, Tail, TailSizeT>> {
    template <typename... Args>
    VlaWrapper(TailSizeT tail_size, Args && ... args) :
        T(std::forward<Args>(args)...), _tail_size(tail_size)
    {}

private:
    TailSizeT _tail_size;
};


Believe it or not, the implementation is a bit more complex than this, but the idea is that, using the wrapper, I can use my class (new_data_base_t in this case) and I also have the features to access VLA data. Unfortunately, the struct I want to create must be changed, but on the other hand there is no need for manual memory management.

Going further, Vla needs to be owned in order to work. I designed a way to create a collection of Vlas inside obj_reader_t::load, which is passed downstream in order to allocate new objects and point to them. This changes the concepts a bit, because new the allocations are performed on the children, with this approach the children will just point to the "arena" of allocations. I already studied how xrefs are handled (they are the major cause of my attempts ;)), I know how to handle the thing (if I looked wherever I needed to, hopefully  ;D). This new approach will address the problem of "memory responsibility" (it will be based on RAII, storing the handlers inside a container), the only downside is that the container itself is an overhead. I was thinking about std::deque, which preserves references validity and it has smaller memory overhead than std::forward_list. I think that the impact of the container will be minimal, but I also want to hear your opinion.

At this point, I think that you understood why I am sharing this idea before diving into the code. I wanted to start smaller, but I started feeling itchy because of these VLAs, and I would really like to get rid of them. Maybe you guys disagree with my approach, maybe you have better ideas. In any case, I think that your opinion is very important in this case, I am not talking about "just smashing 'override's around" this time ;). And I am really using C++11 features for this thing, so it is quite important in this case  :)

Let me know what you think.

P.s.: there are a few things I want to discuss about Vla and VlaWrapper APIs, but for now it is better to talk about the general idea.

prissi

First, the same object obtained from the reader code (the desc_.._t objects) are referenced in various hashtables and lists (up to three). This does not look like a task for a pointer that cannot be copied like unique_ptr. And paks will be never destructed but at the very end of the game. So no need to worry about deallocation at all. Furthermore, every obj has a pointer to its descriptor (and not the descriptor itself, because this would allocate way too much memory).

Honestly, I have read this five times, and I cannot follow your idea. I did not study C++ programming ever at a university level. So I have to google even uncommon abbreviations like RAII. So for me seems this just seems much more complicated than new with a parameter. The reader code is already difficult to follow (especially when the xref_reader kicks in), and this does not look like making things easier. Also I found several remarks that dumb pointer should be preferred for everything than lives beyond a scope, which is exactly the case for all objects reading in a pak.

But again with my confusion (I am happy to learn though). Before I could write

legacy_data_t *d;
d = new (22) legacy_data_t;
// so something with the chars
if(d->data[8]=='A') { ...

and now it would be what? It seems that simple d->data[8] is not working after that?

Or are we just talking on the buffer for reading in the objects, i.e. the ALLOCA macro?

Ters

Quote from: prissi on November 06, 2018, 01:39:57 AMThis does not look like a task for a pointer that cannot be copied like unique_ptr.
How it would be done is that one place "owns the pointers", the others just borrow it "temporarily". That makes it clear who is responsible to freeing the memory. However, for anything in the descriptor directory (but not its subdirectories), there is no need to free the memory, as their life-span is until the death of the process. While not official policy, a veteran Microsoft developer has written that freeing memory (and closing handles) at process exit is just wasting time. (The topic is DLL shutdown callbacks, but the arguments hold true the main executable as well.)

These template things are not exactly easy on the eyes. I also don't see why GCC is creating problems for you. The example I posted earlier worked just fine. There is however a problem with the placement new operator used according to C++14, but that is relatively easy to work around.

Dwachs

I do not get why you want to change a working system. The deallocation of memory could also be achieved writing (virtual) destructors for all these desc-classes.
Parsley, sage, rosemary, and maggikraut.

ceeac

Quote from: dodomorandi on November 05, 2018, 10:02:19 PMUnfortunately, it looks like that GCC stopped allowing me to use C99 VLAs after some C++11 changes

This is because VLAs are illegal in pure C++11 and are only available as a GCC extension. They can be dis-/enabled by the -W(no-)vla flag which is enabled by -Wpedantic.

Ters

Quote from: Dwachs on November 06, 2018, 07:26:09 AMThe deallocation of memory could also be achieved writing (virtual) destructors for all these desc-classes.
Not if the memory is part of the same allocation as the object itself, as it currently is. obj_desc_t also says virtual members are forbidden, which might be because some of the structures are read raw from file. Or maybe they just were read raw once, but all have since been changed to incorporate compatibility code.

dodomorandi

Quote from: prissi on November 06, 2018, 01:39:57 AMFirst, the same object obtained from the reader code (the desc_.._t objects) are referenced in various hashtables and lists (up to three). This does not look like a task for a pointer that cannot be copied like unique_ptr.
As Ters already said, std::unique_ptr (or Vla) is responsible for construction and destruction, you can still point to it. And IMHO std::reference_wrapper<std::unique_ptr<T>> is much more expressive than T** (is it a pointer to a pointer, a pointer to an array, an array of pointers, an array of arrays... are they nullable or not...)

Quote from: prissi on November 06, 2018, 01:39:57 AMHonestly, I have read this five times, and I cannot follow your idea.
Sorry, I tried to focus on concepts instead of deeping into details, maybe it was not clear enough.

Quote from: prissi on November 06, 2018, 01:39:57 AMI did not study C++ programming ever at a university level.
Me neither, I am self taught with C++.

Quote from: prissi on November 06, 2018, 01:39:57 AMSo I have to google even uncommon abbreviations like RAII.
Sorry, but RAII is totally not uncommon -- C++ is the most famous language based on "Resource Acquisition Is Initialization". CRTP is a bit less known acronym, but it is widely used because it is one of few ways to have static inheritance.

Quote from: prissi on November 06, 2018, 01:39:57 AMBut again with my confusion (I am happy to learn though). Before I could write
Codice: [Seleziona]

legacy_data_t *d;
d = new (22) legacy_data_t;
// so something with the chars
if(d->data[8]=='A') { ...

and now it would be what? It seems that simple d->data[8] is not working after that?

I omitted some parts to avoiding details that could be discussed later. With the [t]new_data_t[/t] defined like in my example, the idea is having something like

new_data_t d(22);
if(d.tail_begin()[8]) { ...

http://coliru.stacked-crooked.com/a/6a1c4a084bae4d68 Here you can find the stub implementation and a simple working example (look below line 148). As you can see, using Vla (I had other names in my initial implementation, but it is the same) it is pretty easy, less error prone and, quite important, C++11 compliant.

Quote from: Ters on November 06, 2018, 07:16:51 AMthere is no need to free the memory, as their life-span is until the death of the process.
In this case I need to build a "generic" handler, it would be quite bad to explicitly design it to leak.

Quote from: Ters on November 06, 2018, 07:16:51 AMWhile not official policy, a veteran Microsoft developer has written that freeing memory (and closing handles) at process exit is just wasting time.
I think that this is the opinion of just one person. I would really like to ask Herb Sutter from Microsoft if he shares the opinion. My point is simple: if you leak, you put yourself in the position you cannot refactory the code in order to delete and reuse some memory without changing everything. Moreover, it will probably take some hundreds of milliseconds to drop the memory in simutrans (in the worst case), I doubt that any user would notice it.

Quote from: Ters on November 06, 2018, 07:16:51 AMThese template things are not exactly easy on the eyes.
If you look at my example, in practice you will never use them "inside functions", because you would just simply use a typedef. For more complex things (expecially SFINAE), things have changed with C++17, unfortunately in C++11 we have many ugly template boilerplate in many different situations.

Quote from: Ters on November 06, 2018, 07:16:51 AMI also don't see why GCC is creating problems for you. The example I posted earlier worked just fine. There is however a problem with the placement new operator used according to C++14, but that is relatively easy to work around.
Maybe it's what ceeac said, I will check again what was going on.

Quote from: Dwachs on November 06, 2018, 07:26:09 AMI do not get why you want to change a working system.
As I said, the way it works it is not C++ compliant. The fact that something "just work" does not mean it "must not be improved". A small dumb example:

#include <iostream>
#include <new>

template <typename T> struct A {
  char c;
  T d[];
};

int main() {
  using data_t = char;

  auto a = static_cast<A<data_t> *>(operator new(sizeof(char) +
                                                 sizeof(data_t) * 10));
  a->c = 'a';
  for (unsigned index = 0; index < 10; ++index)
    a->d[index] = static_cast<data_t>(index + 65);

  std::cout << a->c;
  for (unsigned index = 0; index < 10; ++index)
    std::cout << ' ' << a->d[index];
  std::cout << '\n';

  operator delete(a);
}

This works fine. If you change using data_t = char to using data_t = double, the code will trigger UB overriding the stack. And this happens just because in the second case some padding has been inserted before [t]d[/t], and we "just forgot" to calculate the size of the object according to members alignment.

If you don't want to change how things works, fine. As Dwachs said, if things work you don't need to change them. If the idea is to improve the current status of the project, it is possible to discuss about pros and cons of different approaches. If things "work as they are", I don't think it makes sense to discuss any of my C++11 proposal.

Ters

Quote from: dodomorandi on November 06, 2018, 09:23:12 PMI think that this is the opinion of just one person. I would really like to ask Herb Sutter from Microsoft if he shares the opinion. My point is simple: if you leak, you put yourself in the position you cannot refactory the code in order to delete and reuse some memory without changing everything. Moreover, it will probably take some hundreds of milliseconds to drop the memory in simutrans (in the worst case), I doubt that any user would notice it.

Simutrans uses enough memory that some of it may be swapped out, particularly descriptors that are not often used. It may therefore take seconds. (Simutrans players can not be expected to have SSDs and loads of RAM. I only barely have the latter.) Even if the suggestion that good responsiveness to user actions is a good idea is just an opinion, it is one I very much share. If things take longer than a second, users start thinking nothing is happening.

Descriptors also do not leak, as Simutrans does not lose track of the descriptors and memory is being freed exactly when it is no longer used. xrefs may be an exception, as they are temporary placeholders. However, there is no need to make complicated generic solutions to a single problem.

Being able to switch pak set without closing the game would be a good idea, but I think that calls for something else than (just) unique_ptr, as everything in the game expects that a reference to a descriptor, once obtained, will always be valid. Using shared_ptr and/or weak_ptr won't solve the problem either, just morph it into something else. It would also likely have a significant impact on performance.

Quote from: dodomorandi on November 06, 2018, 09:23:12 PMAnd IMHO std::reference_wrapper<std::unique_ptr<T>> is much more expressive than T**
The people behind C++ must have very wide screens and/or use very small fonts. And never have to show code in a presentation. They went from one extreme (short but ambiguous) to the other (precises, but long-winded). It is typical of human kind, but rarely a good idea in hindsight.

DrSuperGood

QuoteI think that this is the opinion of just one person. I would really like to ask Herb Sutter from Microsoft if he shares the opinion. My point is simple: if you leak, you put yourself in the position you cannot refactory the code in order to delete and reuse some memory without changing everything. Moreover, it will probably take some hundreds of milliseconds to drop the memory in simutrans (in the worst case), I doubt that any user would notice it.
Tell that to the 4-6GB extended server game where destructing the map on close takes a good 5-10 seconds. If I close process structure from Task Manager it is as good as instant and does not leak.

The main advantage to clean up the object data leaks would be so that one can introduce pakset reloading inside Simutrans. Currently one has to restart Simutrans in order to change pakset. This would be useful for joining server games as one could select the server and if it is not the currently loaded pakset it could load the correct pakset and then join the game. Could also apply to loading saved games.

prissi

#28
The latter could be achieved by just starting a new instance of simutrans on the command line and while closing the current. It will be even faster than deallocation all the memory ... For saved games, simutrans can detect the pak used on load time.

We added proper freeing of the map some time ago. With our homemade memory allocation routines (which overload new), the time was just bareable, but with regular new/delete the freeing of the map can be close to minutes. The pak have a little less objects, so it may not be that bad.

The thing I personally like about C is more that is like high level assembler. Very little hides the functionality of my code. (On the MC68k, teh C(++) code was almost directly translateable to assembler and vice versa.) Thus I would prefer **T or *T[] much more over std::reference_wrapper<std::unique_ptr<T>> (where I have to look into the cpp reference to find out, what is the meaning). I like the idea of reversible code. Assume the compiler compiles without optimisations: How would a clever decompiler present the code? I almost bet on **T or *T[]. Or, rather following the above example d.text[8] is my choice over d.tail_begin()[8]. The latter reminds me rather of how windows private data access in the GDI. If you really worry on overflow, you could add a size element, which is filled on load time, and over [] for the text, which to me feels more C++ like.

Imho, if one wants foolproof or automatic memory handling, other language are better suited than C++. Being able to do it anyway, does not imply it is a good idea. Like you can use a gun as a hammer. I found the different casts already an eyesore and a pain to type. I rather use my letters for long variable and function names.