News:

Do you need help?
Simutrans Wiki Manual can help you to play and extend Simutrans. In 9 languages.

use of vector_tpl

Started by hreintke, February 03, 2013, 01:17:42 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

hreintke

LS,

Experimenting with search_route I included an additional vector_tpl<halthandle_t> to ware_t in simware.h :


class ware_t
{
friend class warenbauer_t;

private:
/// private lookup table to speedup
static const ware_besch_t *index_to_besch[256];

public:
// added vector to hold route
vector_tpl<halthandle_t> route_halts;

/// type of good, used as index into index_to_besch
uint32 index: 8;


Then I get the compile error :


1>c:\utilities\mingw\msys\1.0\home\herman\simutest\trunk\simware.h(122): error C2248: 'vector_tpl<T>::operator =' : cannot access private member declared in class 'vector_tpl<T>'
1>          with
1>          [
1>              T=halthandle_t
1>          ]
1>          c:\utilities\mingw\msys\1.0\home\herman\simutest\trunk\dataobj\../tpl/vector_tpl.h(266) : see declaration of 'vector_tpl<T>::operator ='
1>          with
1>          [
1>              T=halthandle_t
1>          ]
1>          This diagnostic occurred in the compiler generated function 'ware_t &ware_t::operator =(const ware_t &)'


Are there limitations on the usage of vector_tpl<halthandle_t> ?

Further trials gave result that it works when moving


vector_tpl& operator=( vector_tpl const& other ) {
vector_tpl tmp(other);
swap(tmp, *this);
return *this;
}


from the private to the public part of the template.

But this looks like a quite drastic update.

Herman

Dwachs

You will need to add such an operator to the vector_tpl. This is necessary as assignments of ware_t instances are very frequent in the code.

Why do you want to store the route in the ware_t structure?
Parsley, sage, rosemary, and maggikraut.

hreintke

QuoteYou will need to add such an operator to the vector_tpl

I don't understand what you mean with this.
The = operator is in vector_tpl, but in the "private part".
I moved from private to public and then it worked.

But I don't understand why is doesn't work without that. vector_tpl<halthandle_t> is used multiple times in the code.

For the usage : I am experimenting in getting/showing more route information.
Regulary I have goods which travel across halts I did not anticipate (currently f.e multiple coal, iron and steel) as they all use the same trains/trucks. A first try is saving the route information already gathered by search_route in the ware_t for further usage.

Herman

Dwachs

Quote from: hreintke on February 03, 2013, 02:31:16 PM
I don't understand what you mean with this.
The = operator is in vector_tpl, but in the "private part".
I moved from private to public and then it worked.

But I don't understand why is doesn't work without that. vector_tpl<halthandle_t> is used multiple times in the code.
If you write

ware_t w = another_w;

then the contents of all member variables of ware_t need to be copied. Hence you need this copy operator for vector_tpl. If this is declared in the 'private' part then it cannot be used from outside. The implementation with swap is only safe for simple data types. It works for halthandle_t, though.

vecotr_tpl<halthandle_t> is used in the code, but never copied as such.
Parsley, sage, rosemary, and maggikraut.

Ters

Private assignment operators and/or copy constructors is a normal way of saying "don't ever make copies of instances of this class!", though then there usually isn't any implementation of it. Adding a vector to ware_t sounds like a potential killer for performance. ware_t has apparently been greatly optimized for size, down to every single bit. As I understand it, there can be many, many thousands of ware_t instances.

hreintke

OK, thanks.

Another day of learning on the job.
I think I understand now why the vector_tpl cannot be used as I thought in ware_t.

@Ters : I know ware_t changes should be carefully evaluated due to the heavy use.
But I am working on understanding working of ST and the implementation in the code.
That means I have private patches not optimized for performance but just helping.

If moved to "proposed patches" additional work is (to be)done to keep all  also performance wise workable.

Herman

prissi

#6
Since a route search is usually less than 12 hops (or you just require it), you may get better performance using a route like
halthandle_t halt_route[12];
That is only 24 bytes, and due to the size is known, it is probably not much slower than a simple ware_t.

By the way, maybe ware_t should use freelist_t, as after trees is is probably the most requested thing on the map. That could help too.

hreintke

LS,

I am again struggling to get a vector_tpl working.

Now I have defined a class :


class input_goods_t {
public :
uint8 input_index;
uint32 usage;
bool optional;
input_goods_t(const uint8 idx, const uint32 usg, const bool opt) : input_index(idx), usage(usg), optional(opt) {};
input_goods_t() :input_index(0),usage(100),optional(false){};
};


and wanted to create a vector of these is ware_production_t.


vector_tpl<input_goods_t> component_goods;


But when acccessing the vector I get the compile time error :


1>c:\utilities\mingw\msys\1.0\home\herman\simutest\trunk\gui\../simfab.h(145): error C2248: 'vector_tpl<T>::operator =' : cannot access private member declared in class 'vector_tpl<T>'
1>          with
1>          [
1>              T=input_goods_t
1>          ]


Even when doing a test with a simple type vector like


vector_tpl<uint32> test;


I get a simular error.

But when I "include <vector>"
And define component_goods as

std::vector<input_goods_t> component_goods;

I can make use of it with no problems

ausgang[0].component_goods.push_back(input_goods_t(0,100,false));


Questions
- what causes the vector_tpl to be not usable in this case ?
- Can i use the std::vector instead of vector_tpl without causing other issues ?

Herman


prissi

If you do this in an header file, the vector class might not be fully available, and just a forward declaration. However, I would expect other errors in this case.

Are you using vector_tpl inside a class, which is then put into another vector or is assigned during calls?

Without showing the code in question diagnosis is difficult. But generally you are not allowed to copy a vector since it is a bad idea performance wise and (just image a vector of pointers) has a high chance of causing troubles later on.

Dwachs

Copy-assignment and copy-constructor are not implemented at all and set to private to prevent the compiler to use default implementations, which caused trouble if the data type in the vector is not a simple one.

You have to implement it yourself...

Maybe you can work with references of vectors instead of copying them for return values or parameters?
Parsley, sage, rosemary, and maggikraut.

hreintke

#10
LS,

I used it in simfab.h where I added the vector_tpl to ware_production_t

code snippet is :

#define DEMAND_BITS (4)

// added class by HR
class input_goods_t {
public :
uint8 input_index;
uint32 usage;
bool optional;
input_goods_t(const uint8 idx, const uint32 usg, const bool opt) : input_index(idx), usage(usg), optional(opt) {};
input_goods_t() :input_index(0),usage(100),optional(false){};
};


/**
* Convert internal values to displayed values
*/
sint64 convert_goods(sint64 value);
sint64 convert_power(sint64 value);
sint64 convert_boost(sint64 value);

// to prepare for 64 precision ...
class ware_production_t
{
private:
const ware_besch_t *type;
// Knightly : statistics for each goods
sint64 statistics[MAX_MONTH][MAX_FAB_GOODS_STAT];
sint64 weighted_sum_storage;

/// clears statistics, transit, and weighted_sum_storage
void init_stats();

public:
ware_production_t() : type(NULL), menge(0), max(0), index_offset(0)
{
init_stats();
};

const ware_besch_t* get_typ() const { return type; }
void set_typ(const ware_besch_t *t) { type=t; }

// Knightly : functions for manipulating goods statistics
void roll_stats(sint64 aggregate_weight);
void rdwr(loadsave_t *file);
const sint64* get_stats() const { return *statistics; }
void book_stat(sint64 value, int stat_type) { assert(stat_type<MAX_FAB_GOODS_STAT); statistics[0][stat_type] += value; }
void set_stat(sint64 value, int stat_type) { assert(stat_type<MAX_FAB_GOODS_STAT); statistics[0][stat_type] = value; }
sint64 get_stat(int month, int stat_type) const { assert(stat_type<MAX_FAB_GOODS_STAT); return statistics[month][stat_type]; }

/**
* convert internal units to displayed values
*/
sint64 get_stat_converted(int month, int stat_type) const {
assert(stat_type<MAX_FAB_GOODS_STAT);
sint64 value = statistics[month][stat_type];
if (stat_type==FAB_GOODS_STORAGE  ||  stat_type==FAB_GOODS_CONSUMED) {
value = convert_goods(value);
}
return value;
}
void book_weighted_sum_storage(sint64 delta_time);

// ADDED vector
std::vector<input_goods_t> component_goods;
vector_tpl<input_goods_t> comp_goods;

sint32 menge; // in internal units shifted by precision_bits (see step)
sint32 max;
sint32 transit;

uint32 index_offset; // used for haltlist and lieferziele searches in verteile_waren to produce round robin results
};


I do get compile errors when compiling simfab.cc, others like
fabrikbauer.cc,  factory_reader.cc,  field.ccgebaeude.cc, leitung2.cc, dings\leitung2
which also include simfab.h compile OK.

Even if I do not reference anything of the added vector. I get the error. So the isue must be in the definition.

In simfab.h little later in the definition of fabrik_t a simular construct is used (standard code) with no issues.


struct field_data_t
{
koord location;
uint16 field_class_index;

field_data_t() : field_class_index(0) {}
explicit field_data_t(const koord loc) : location(loc), field_class_index(0) {}
field_data_t(const koord loc, const uint16 class_index) : location(loc), field_class_index(class_index) {}

bool operator==(const field_data_t &other) const { return location==other.location; }
};
vector_tpl <field_data_t> fields;


@Dwachs :
1/ I do not even use the vector and I alrady get the error.
2/ Could I implement just for my new vector or do I need to implement for all possibilities ? Can you point me to an example ?

Edit ->

I noticed this additional line from the compiler :

This diagnostic occurred in the compiler generated function 'ware_production_t &ware_production_t::operator =(const ware_production_t &)'


So I suppose I have to create my own ware_production_t::operator = implementation.

Can't find any other place in the code where a simular thing is done : do you know of that so I can see what is needed

Dwachs

The problem is, that ware_production_t itself is used as template argument in some array_tpl<ware_production_t>. The array-tpl methods (see eg resize) need the copy assignment of ware_production_t, which itselfs tries to use the (private) copy assignment of vector_tpl<>. Error.

You could solve this by hand-crafting an copy-assignment operator for ware_production_t.

What are you trying to achieve?
Parsley, sage, rosemary, and maggikraut.

hreintke

Thanks Dwachs.

Now I understand why and when this happens and try to find a way around.

I am experimenting with adding functionality to the production strategy of factories.
Now for each of the products all input goods are required. I am looking for a way to define and use partial (or optional) inputgoods.
In first tries I mostly use something like vector as it easliy adds elements and looping thru.

BTW, is there a specific (historical ?) reason for simutrans own implementation of vector/array... templates instead of using the std library ? Would usage of std::vector lead to other issues when using that in a patch ?

Herman

Ters

Whatever the reason is for Simutrans having it's own vector_tpl (possibly bad or lacking std classes on some target platforms), using std::vector will suffer from the same performance issues when nesting it inside another container. The only thing is that std::vector might support swapping as an alternative to copying, but I haven't looked much into this. But for swapping to work, I think it will have to be supported by outer container, the class contained in outer container (if not just the inner container itself), and the inner container container in that class.

Dwachs

We also have a swap method, it is however low-level data swapping.
Parsley, sage, rosemary, and maggikraut.

Ters

It looks like it is the same swap concept, but it is not used when resizing vector_tpl and array_tpl. Whether std::vector uses swap, copy construction or copy assignment seems to be implementation specific and C++ version specific. The new C++ standard apparently introduced move semantics. Previous two sentences is just based on a quick google.

prissi

vector_tpl stems from 1997, when std:: was not even yet a standard (or to be pendantic, it was a standard but almost not implemented). Also our slist_tpl has only one pointer, which is an issue especially for small stuff.

But as Ters said, copying stuff with pointers could be very dangerous; swapping may work, but C++ = is a copy. Thus even a public swap of vector_tpl will not be used by the compiler in those cases imho.

Ters

Compilers won't automatically use std::swap, it has to be called explicitly I believe. And if no custom std::swap is written for swapping a certain class, the default implementation likely uses copy assignment. (There is a custom std::swap for std::vector, but that won't help if the std::vector is inside another class.) However the GNU C++ library is so complicated that I can't tell how its vector implementation moves data when it needs a new and bigger internal buffer.