I can't get SVN rev. to compile with Mingw/GCC

[Spike]

$ svn up
Updating '.':
At revision 5282.


gcc version 3.4.4 (cygming special, gdc 0.12, using dmd 0.125)

===> CXX besch/reader/obj_reader.cc
besch/reader/../../dataobj/../tpl/hashtable_tpl.h: In copy constructor `hashtabl
e_tpl<obj_type, stringhashtable_tpl<slist_tpl<obj_besch_t**> >, inthash_tpl<obj_
type> >::node_t::node_t(const hashtable_tpl<obj_type, stringhashtable_tpl<slist_
tpl<obj_besch_t**> >, inthash_tpl<obj_type> >::node_t&)':
besch/reader/../../dataobj/../tpl/hashtable_tpl.h:266:   instantiated from `bool
hashtable_iterator_tpl<key_t, value_t, hash_t>::next() [with key_t = obj_type,
value_t = stringhashtable_tpl<slist_tpl<obj_besch_t**> >, hash_t = inthash_tpl<o
bj_type>]'
besch/reader/obj_reader.cc:353:   instantiated from here
besch/reader/../../dataobj/../tpl/stringhashtable_tpl.h:52: error: `stringhashta
ble_tpl<value_t>::stringhashtable_tpl(const stringhashtable_tpl<value_t>&) [with
value_t = slist_tpl<obj_besch_t**>]' is private
besch/reader/../../dataobj/../tpl/hashtable_tpl.h:266: error: within this contex
t
besch/reader/../../dataobj/../tpl/hashtable_tpl.h: In copy constructor `hashtabl
e_tpl<const char*, slist_tpl<obj_besch_t**>, stringhash_t>::node_t::node_t(const
hashtable_tpl<const char*, slist_tpl<obj_besch_t**>, stringhash_t>::node_t&)':
besch/reader/../../dataobj/../tpl/hashtable_tpl.h:266:   instantiated from `bool
hashtable_iterator_tpl<key_t, value_t, hash_t>::next() [with key_t = const char
*, value_t = slist_tpl<obj_besch_t**>, hash_t = stringhash_t]'
besch/reader/obj_reader.cc:356:   instantiated from here
besch/reader/../../gui/components/../../tpl/slist_tpl.h:433: error: `slist_tpl<T
>::slist_tpl(const slist_tpl<T>&) [with T = obj_besch_t**]' is private
besch/reader/../../dataobj/../tpl/hashtable_tpl.h:266: error: within this contex
t
common.mk:49: recipe for target `build/default/besch/reader/obj_reader.o' failed

It hinders me in development. Is there something I can do about it on my side? I don't understand the templates at my current skill level of C++ good enough to fix that, and I don't even know a workaround. I'd like to work on Simutrans though, and test my changes.

[Ters]

I think it has to do with this: http://forum.simutrans.com/index.php?topic=8994.0

But I have no problems compiling r5282 using gcc 4.5.2.

[Spike]

Ok, then it might be my own patches. Don't know what to do about it at the moment though, since I'm pretty sure I have not touched the template files themselves.

[isidoro]

Just in case it helps, I think you are using quite an old version of gcc (3.4.4)...  Maybe that's the problem...

Another source of problems may be that you touched headers files to make the code compile faster.  One has to be careful when templates are involved since it can be necessary to instantiate them several times.

Template errors tend to be very verbose, but they are not very difficult to read.  I don't have the code at hand, but it seems that the problem happens from line 353 of obj_reader.cc.  A copy constructor is involved, but it is private and not accessible.  Here is what STL decryptor formats for the first one:

Code Select Expand


BD Software STL Message Decryptor v3.10 for gcc 2/3/4
===> CXX besch/reader/obj_reader.cc

besch/reader/../../dataobj/../tpl/stringhashtable_tpl.h:52: error:
   `stringhashtable_tpl<
        slist_tpl<obj_besch_t **>
    >::stringhashtable_tpl(
        const stringhashtable_tpl<slist_tpl<obj_besch_t **> > &
    )' is private
besch/reader/../../dataobj/../tpl/hashtable_tpl.h:266: error: within this
    context
besch/reader/../../dataobj/../tpl/hashtable_tpl.h: In copy constructor
   `hashtable_tpl<
        obj_type, stringhashtable_tpl<slist_tpl<obj_besch_t **> >
      , inthash_tpl<obj_type>
    >::node_t::node_t(
        const hashtable_tpl<
            obj_type, stringhashtable_tpl<slist_tpl<obj_besch_t **> >
          , inthash_tpl<obj_type>
        >::node_t &
    )':
besch/reader/obj_reader.cc:353:   instantiated from here
besch/reader/../../dataobj/../tpl/hashtable_tpl.h:266:   instantiated from
   `bool hashtable_iterator_tpl<
        obj_type, stringhashtable_tpl<slist_tpl<obj_besch_t **> >
      , inthash_tpl<obj_type>
    >::next()'
besch/reader/obj_reader.cc:353:   instantiated from here


Try to compile a fresh r5282 release and see if it is either the compiler or your code...

[Dwachs]

These are precisely the compile errors when I was developing the patch for the issue raised here:
http://forum.simutrans.com/index.php?topic=8994.0

My guess is that there is a deviation from simutrans-standard in one of these files: tpl/slist_tpl.h,  the hashtable templates in tpl/, or in besch/reader/obj_reader.cc.

Try to use these files directly taken from standard and see if it compiles.

[ras52]

This error is genuine, and it is one of those slightly peculiar situations where the C++ standard permits the compiler to call the copy constructor but doesn't require it.  Hans's compiler is older and so doesn't optimise out the copy constructor call; most of the rest of us are using newer ones that do remove it.  Neither compiler is at fault.

The problem stems from the fact that obj_reader::loaded and obj_reader::unresolved are hashtables whose value type is itself a hashtable.  Any code that calls a method on these that causes the value to be copied should give a compile error.  This includes the get (almost certainly due to a bug), set, remove, and the two-argument form of put.  However the code isn't calling those methods.  The function that is being called is next() on an iterator over those hash tables.  On line 266 of hashtable_tpl.h we construct an slist_iterator_tpl<T> where T is a non-copyable type.  In itself that's fine, though as the slist_iterator contains a slist::node_t which itself contains a T, the iterator is non-copyable.  However the slist_iterator's copy assignment operator doesn't try to copy the value, only the node pointer (see comment in slist_tpl.h, line 465).  The result is that line 266 of hashtable_tpl is effectively doing: X x; x = X(); where X has a working public copy-assignment operator, but no copy constructor.  Because X() is a temporary, the C++ standard permits (but doesn't require) it to be copied before it is bound to the assignment operator parameter. (See section 8.5.3, paragraph 5, second bullet point, first sub-bullet point, of the 1998 edition of the C++ standard if you want the gory details.)

[Dwachs]

Thanks for looking into this. Now I have to decode your answer

The gcc3-version of the nightly builds failed with the same error.

Edit: So one of the following two things would cure the compile error ?
(1) provide a copy constructor of slist_iterator_tpl<T> acting similarly as the copy-assignment operator
(2) in line 266 of hashtable_tpl: replace the assignment by (a) constructing an auxiliary iterator (invokes constructor), then (b) force to use copy-assignment

I do not have gcc3 here to test whether it would work.

[Ters]

It is also possible, and possibly much better, to not have a node instance in the iterator. One might as well replace it with a pointer to head in the list being iterated. That might break some code relying on side effects of the way the iterator works now. Just having the next pointer for the node rather than the node should also, work, but requires changing how the current pointer works (it must start as NULL, not as &lead).

Both solutions should reduce the size of the iterator also.

[Dwachs]

@Ters: I tried this but it did not work, as iterator.next() is called before accessing the current iterate. So there is a need for a predecessor node.

[Ters]

That's why current must start as NULL. When next() is first called, it will see that current is NULL and set it equal to the pointer to the first node. Once current is no longer NULL, further calls to next() will set current to current->next.

[Dwachs]

I committed changes in r5284, which follow your suggestions. Special thanks to ras52 for pointing to the actual error.

Please anybody with access to gcc version 3 try to compile recent trunk.

[TurfIt]

r5285, gcc version 3.4.5 (mingw-vista special r3)

Code Select Expand


===> CXX dings/baum.cc
dings/baum.cc: In static member function `static bool baum_t::alles_geladen()':
dings/../tpl/weighted_vector_tpl.h:418: error: `weighted_vector_tpl<T>::weighted
_vector_tpl(const weighted_vector_tpl<T>&) [with T = uint32]' is private
dings/baum.cc:272: error: within this context
make: *** [build/default/dings/baum.o] Error 1


[Dwachs]

thank you

this has to wait till tomorrow 

[ras52]

Sorry for going AWOL this afternoon.  Either of Dwachs' earlier suggestions would, I imagine, fix the problem.  (Though I don't have a copy of gcc 3 readily accessible to test with either.)  Ters' suggestion of removing the node from the iterator has to be a good one, and the way Dwachs has done this looks good to me. 

I have to say, the whole slist_iterator_tpl interface seems counter-intuitive to me.  It's certainly not a standard C++ iterator interface, but neither is it quite the Java interface, despite looking similar to it.  next() returns a bool not a value, for example.  I'm sure we would be well served by deprecating slist_iterator_tpl in favour of slist_tpl::iterator and ::const_iterator.

The underlying problem remains unfixed: namely that containers of containers are extremely fragile.  The real solution is probably either to remove all containers of containers, or to make containers copyable.

TurfIt's error in baum.cc looks much the same. weighted_vector_tpl has a copy assignment operator that looks truly broken, though will certainly compile; however it has a private unimplemented copy constructor.  The older compiler wants to make a copy of the default-constructed temporary weighted_vector_tmpl on line 272 of baum.cc; the newer compiler binds the temporary directly to the parameter.

[Dwachs]

I have to say, the whole slist_iterator_tpl interface seems counter-intuitive to me.  It's certainly not a standard C++ iterator interface, but neither is it quite the Java interface, despite looking similar to it.  next() returns a bool not a value, for example.  I'm sure we would be well served by deprecating slist_iterator_tpl in favour of slist_tpl::iterator and ::const_iterator.

The problem here is, that slist_tpl::iterator works on non-const lists of non-const nodes, slist_tpl::const_iterator works on const-lists of const-nodes, whereas slist_iterator_tpl works on const lists of non-const nodes.

[Dwachs]

Please try again with gcc3 to compile r 5289. I now made the copy-assignment of weighted_Vector_tpl private, copying of vectors is not well supported I guess.

[ras52]

That patch looks good to me.  (Though I don't actually have a gcc 3 to try it on.)

[Dwachs]

@Hans: I moved the topic to this board just to ask whether iron-bite + current trunk (rev >= 5289) compiles.

[TurfIt]

r5305, gcc version 3.4.5 (mingw-vista special r3)

Code Select Expand


===> CXX gui/components/gui_flowtext.cc
gui/components/../../tpl/slist_tpl.h: In constructor `slist_tpl<T>::const_iterat
or::const_iterator(const slist_tpl<T>::iterator&) [with T = gui_flowtext_t::node
_t]':
gui/components/gui_flowtext.cc:184:   instantiated from here
gui/components/../../tpl/slist_tpl.h:95: error: `slist_tpl<gui_flowtext_t::node_
t>::node_t*slist_tpl<gui_flowtext_t::node_t>::iterator::ptr' is private
gui/components/../../tpl/slist_tpl.h:117: error: within this context
gui/components/../../tpl/slist_tpl.h: In constructor `slist_tpl<T>::const_iterat
or::const_iterator(const slist_tpl<T>::iterator&) [with T = gui_flowtext_t::hype
rlink_t]':
gui/components/gui_flowtext.cc:306:   instantiated from here
gui/components/../../tpl/slist_tpl.h:95: error: `slist_tpl<gui_flowtext_t::hyper
link_t>::node_t*slist_tpl<gui_flowtext_t::hyperlink_t>::iterator::ptr' is privat
e
gui/components/../../tpl/slist_tpl.h:117: error: within this context
make: *** [build/default/gui/components/gui_flowtext.o] Error 1


[Dwachs]

ok, back moving back, thanks for testing

Edit: please check again with r 5306.

I still have hope that we manage to make it compile with gcc3 again *lol*

[Spike]

@Hans: I moved the topic to this board just to ask whether iron-bite + current trunk (rev >= 5289) compiles.

It compiles, but I did like > 100 distributed changes to places which use templates. Generally I have replaced a lot of value types in templates with reference/pointer types, to work around the copy constructor problem. I feel quite uncertain if this will harm performance, and it will lead to higher heap fragmentation, but maybe it's not so bad.

I have to say, the whole slist_iterator_tpl interface seems counter-intuitive to me.  It's certainly not a standard C++ iterator interface, but neither is it quite the Java interface, despite looking similar to it.  next() returns a bool not a value, for example.

It was not possible to rebuild the Java interface in C++, because C++ allows value types in templates and you can't return anything like <null> for a value type. So I had to split the check for end of the iteration (next()) from element access (get_/access_current()). That was the closest I could get to the Java enumeration idea.

I'm a big Java fan, and just didn't want to learn or use the STL. So in the old code you could find a lot of rebuilt Java idioms. Particularly in the UI code there are still some.

[TurfIt]

Edit: please check again with r 5306.

Same. Try again. 

[Dwachs]

Please check again with r 5308.

Honestly, I do not understand why r5308 should make a difference. Either r5306 should compile with all compilers or fail with all :/

[TurfIt]

r5308 makes 3.4.5 happy, for whatever reason. 

[Dwachs]

.. which makes me even more happy