[5802] compile problem

Questions, problems and discussion about compiling FreeOrion.

Moderator: Oberlus

Message
Author
User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: [5802] compile problem

#16 Post by Geoff the Medio »

Vezzra wrote:Well, the errors seem to be different ones, although more.
What happens if you remove the "typename" from lines 39, 46, and 67 of ObjectMap.h (where it has the "error: expected nested-name-specifier before 'const'" listed). I get compile errors without them, but googling suggests they should be removed?
Bigjoe5 wrote:...I'd prefer to clean up my own mess...
OK, though several of my changes are probably worth doing regardless of the form taken by your fixes for the compile issues.

User avatar
Dilvish
AI Lead and Programmer Emeritus
Posts: 4768
Joined: Sat Sep 22, 2012 6:25 pm

Re: [5802] compile problem

#17 Post by Dilvish »

Geoff the Medio wrote:
Vezzra wrote:Well, the errors seem to be different ones, although more.
What happens if you remove the "typename" from lines 39, 46, and 67 of ObjectMap.h (where it has the "error: expected nested-name-specifier before 'const'" listed). I get compile errors without them, but googling suggests they should be removed?
This lead to a fix for me - joy!

It looks like I get pretty much the same errors as Vezzra with your base patch, Geoff. Removing the 'typename' gives me very specific errors about its absence; if I instead move the 'const' to be before the 'typeneme' in those 3 lines then it seems to get past that ok. Then wind up with ~10 errors of what looks like 3 types
-- two instances of "conversion of scalar value_iterator to non-scalar type const_value_iterator ", triggered by Empire.cpp 1776 and 2597
-- four instances of "no matching function for call to ‘ObjectMap::const_value_iterator<System>::const_value_iterator(std::map<int, System*>::const_iterator)’", triggered by ObjectMap.h 299, 303, 315 and 319
-- four other nominal errors from the same ObjectMap.h 299, 303, 315 and 319 essentially because the return statement was considered invalid because of the above. These latter 8 errors all seem to be gotten around by taking out hte ampersand in line 80, the const_value_iterator constructor "const_iterator& base" --> "const_iterator base" . The first couple errors (plus the same thing later appearing in SidePanel.cpp) I got past by just relying on the new constructor: instead of

Code: Select all

for (ObjectMap::const_value_iterator<System> sys_it = Objects().begin_values<System>(); sys_it != Objects().end_values<System>(); ++sys_it)
using

Code: Select all

for (ObjectMap::const_value_iterator<System> sys_it = Objects().begin<System>(); sys_it != Objects().end<System>(); ++sys_it)
that solved all the compile problems and the game seems to run fine also. I thought about making that reliance on the new constructor explicit, but just left it implicit. My cumulative patch vs 5813 (Geoff's plus my additions) is attached. Geoff, a heads-up -- for some reason your patch applied fine to ObjectMap.h, but rejected all 5 chunks for ObjectMap.cpp for me, but when I went to manually apply the changes it looked to my eye like the patch chunks all should have matched up fine. So you may have a little hiccup when applying this and just take out the ampersand, mentioned above, manually.
Attachments

[The extension patch has been deactivated and can no longer be displayed.]

If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: [5802] compile problem

#18 Post by Geoff the Medio »

Dilvish wrote:...taking out hte ampersand in line 80, the const_value_iterator constructor "const_iterator& base" --> "const_iterator base" .
What happens if you make ObjectMap.h line 80:

Code: Select all

const_value_iterator(const typename std::map<int, T*>::const_iterator& base) :
? I don't see why it shouldn't be possible to pass the parameter by reference.

Code: Select all

for (ObjectMap::const_value_iterator<System> sys_it = Objects().begin<System>(); sys_it != Objects().end<System>(); ++sys_it)
??? That doesn't make much sense...

User avatar
Vezzra
Release Manager, Design
Posts: 6095
Joined: Wed Nov 16, 2011 12:56 pm
Location: Sol III

Re: [5802] compile problem

#19 Post by Vezzra »

Dilvish wrote:This lead to a fix for me - joy!
Ok, I finally came around to test this patch on OSX, success! Everything compiled just fine, and I was able to play a test game beyond turn 100 without issues.
Geoff, a heads-up -- for some reason your patch applied fine to ObjectMap.h, but rejected all 5 chunks for ObjectMap.cpp for me, but when I went to manually apply the changes it looked to my eye like the patch chunks all should have matched up fine. So you may have a little hiccup when applying this and just take out the ampersand, mentioned above, manually.
The reason for that is that ObjectMap.cpp has Windows style line endings (CR/LF). When I changed the line endings in ObjectMap.cpp to LF, and also those in the patch file, the patch applied fine.
Geoff the Medio wrote:What happens if you make ObjectMap.h line 80:

Code: Select all

const_value_iterator(const typename std::map<int, T*>::const_iterator& base) :
? I don't see why it shouldn't be possible to pass the parameter by reference.
That also compiles without errors on OSX. I continued my test game with the recompiled app for a few turns, no issues.

So I think you can commit these fixes.

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: [5802] compile problem

#20 Post by Geoff the Medio »

Vezzra wrote:So I think you can commit these fixes.
Bigjoe5 has other plans...

User avatar
Bigjoe5
Designer and Programmer
Posts: 2058
Joined: Tue Aug 14, 2007 6:33 pm
Location: Orion

Re: [5802] compile problem

#21 Post by Bigjoe5 »

Geoff the Medio wrote:
Dilvish wrote:...taking out hte ampersand in line 80, the const_value_iterator constructor "const_iterator& base" --> "const_iterator base" .
What happens if you make ObjectMap.h line 80:

Code: Select all

const_value_iterator(const typename std::map<int, T*>::const_iterator& base) :
? I don't see why it shouldn't be possible to pass the parameter by reference.
My suspicion is that it's taking a map<int, T*>::iterator, and converting it to a map<int, T*>::const_iterator, thus creating a new temporary variable which is then being passed to the constructor.

Such conversions are probably the cause of some of the other issues we're having as well - for instance, why can't we return an std::pair<int, const T*>& by reference by returning std::map<int, T*>::const_iterator::operator*()? After all, std::map<int, T*>::const_iterator*() is obviously returning by reference. The reason is that it's implicitly converting the const std::pair<const int, T*> to a new local variable of type std::pair<int, const T*>, and attempting to return it by reference leads to disaster.

There are two possibilities that I like - one is to override the increment and decrement operators for the const_iterator to store a version of the pair as an std::pair<int, const T*>, then return a reference or pointer to it in the * and -> operators (seems to be working - and I made sure to include a call to post-increment somewhere, just to instantiate that template function and make sure it compiles/functions correctly). We definitely need to have an intermediate variable somewhere - the question is really whether it's a local variable that we hack into the type we want when we want to return a pointer to it, or a persistent variable that we store as the type we want, and can return a pointer to without difficulty.

The second possibility (the Gordian approach) is to get rid of iterator and const_iterator completely. The value iterators (which could then be renamed iterator and const_iterator) should suffice entirely for anything anyone outside the object map wants to do, since the object's ID is part of the object itself. For stuff within the object map that requires the key, there's plain old std::map::iterator/const_iterator. This is a bit more work, and I haven't tried/probably won't try it out immediately...

As for the the issue with not being able to convert from map iterators to object map iterators, I agree that making explicit constructor calls and just using the plain old map iterator where appropriate as Geoff has done is most appropriate.

As for this issue:
Dilvish wrote:-- two instances of "conversion of scalar value_iterator to non-scalar type const_value_iterator ", triggered by Empire.cpp 1776 and 2597
Neither of these things exactly jive with the error message you're getting, but two perhaps related things:
1. A call to begin_values on a non-const ObjectMap will return an ObjectMap::iterator, which you're trying to assign to a variable of type ObjectMap::const_iterator (which should work...?).
2. Geoff's new implementation of the templated begin and end methods are actually calling the const_iterator's constructor (revealing another flaw in my design - a const_iterator is implicitly convertible to an iterator!)

Aside from the static casts and the #2 issue above, another thing I noticed about that patch was that the -> operator is overloaded to return the same type as the * operator, whereas it would typically return a pointer to that type (which is admittedly not useful in this case - hence the semantics I had in place of treating it as an iterator over a collection of T, which can be treated as a T*. Now it can be treated as a T* in some cases, and a T** in others...).

I'm putting the finishing touches on a patch that should hopefully solve everyone's problems. If it fails, feel free to commit Dilvish's updated version of Geoff's patch while I continue to work on a solution that doesn't involve hacky static casts.
Warning: Antarans in dimensional portal are closer than they appear.

User avatar
Dilvish
AI Lead and Programmer Emeritus
Posts: 4768
Joined: Sat Sep 22, 2012 6:25 pm

Re: [5802] compile problem

#22 Post by Dilvish »

ok, welll I got interrupted alot while testing Geoff's code and writing this message, so there's a lot of new response now. I'm going to go ahead and post this, but it's not responsive to anything after Geoff's post that I quote, if I try to catch up to the new stuff by the time I'm done I'm sure there will just be more posts to adjust to :lol:
Geoff the Medio wrote:What happens if you make ObjectMap.h line 80:

Code: Select all

const_value_iterator(const typename std::map<int, T*>::const_iterator& base) :
? I don't see why it shouldn't be possible to pass the parameter by reference.
ah yes ok, the 'const' before the typename does allow it to compile fine that way, I guess taking out the reference nature had just made it easier to automatically do a conversion.

Code: Select all

for (ObjectMap::const_value_iterator<System> sys_it = Objects().begin<System>(); sys_it != Objects().end<System>(); ++sys_it)
??? That doesn't make much sense...
Well, it seems pretty closely related to the above, which things it could convert and which it couldn't. I guess I wasn't clear enough in my explanation -- my understanding is it's able to convert Objects().begin<System>() ( a standard std::map<int, T*>::iterator ) into a ObjectMap::const_value_iterator<System> because that standard iterator was exactly what was called for by the ObjectMap::const_value_iterator constructor, whereas trying to feed it an ObjectMap::value_iterator was apparently different enough to cause trouble. That made a little sense to me, but sure, there's lots about this whole deal that I wish made more sense. If I now (with the language yoou just suggested) change one of those loops (the SidePanel one) back to using begin_values() and end_values(), I get the error, like before,

Code: Select all

/home/willgohs/Downloads/freeorion_head/FreeOrion/UI/SidePanel.cpp: In member function ‘void SidePanel::RefreshImpl()’:
/home/willgohs/Downloads/freeorion_head/FreeOrion/UI/SidePanel.cpp:2300:90: error: conversion from ‘ObjectMap::value_iterator<System>’ to non-scalar type ‘ObjectMap::const_value_iterator<System>’ requested
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

User avatar
Bigjoe5
Designer and Programmer
Posts: 2058
Joined: Tue Aug 14, 2007 6:33 pm
Location: Orion

Re: [5802] compile problem

#23 Post by Bigjoe5 »

Dilvish wrote:...my understanding is it's able to convert Objects().begin<System>() ( a standard std::map<int, T*>::iterator ) into a ObjectMap::const_value_iterator<System> because that standard iterator was exactly what was called for by the ObjectMap::const_value_iterator constructor, whereas trying to feed it an ObjectMap::value_iterator was apparently different enough to cause trouble.
What's really weird is that Objects().begin<System>() isn't a standard std::map<int, T*>::iterator. It's an ObjectMap::iterator, which shouldn't be any less different from std::map<int, T*>::iterator (or const_iterator) than ObjectMap::value_iterator is.
Warning: Antarans in dimensional portal are closer than they appear.

User avatar
Bigjoe5
Designer and Programmer
Posts: 2058
Joined: Tue Aug 14, 2007 6:33 pm
Location: Orion

Re: [5802] compile problem

#24 Post by Bigjoe5 »

- Eliminated the returning-address-to-temporary problem by storing the value in a local variable
- Eliminated the problem of being able to implicitly convert from a const_iterator to an iterator by making const_iterator derive from map's const_iterator and iterator derive from map's iterator.
- Hopefully eliminated the problems with implicit conversions by making explicit constructor calls, changing some uses of ObjectMap::iterator to std::map::iterator, and adding a constructor taking an std::map::iterator to the const_iterator and const_value_iterator structs.

Pros:
- Solves the implicit conversion from const_iterator to iterator problem.
- Doesn't use a call to the const_iterator constructor to return an iterator.
- Avoids using static casts to return a pointer to a pair<int, const T*>

Cons:
- The member variable I added to the struct doesn't follow FO's naming conventions.
- None of the constructors take reference parameters (I avoided including any changes from Geoff's patch that weren't obviously solving a problem, and in fact were perceived to cause more problems)
- There's not a consensus on whether or not value_iterator should be treated as a T* or a T**.

Regardless of whether or not this patch works, some essential features from each patch are missing from the other, and will have to be added in later - I didn't want to add any non-essentials to this patch, out of caution of unknowingly breaking something else.
Attachments

[The extension patch has been deactivated and can no longer be displayed.]

Warning: Antarans in dimensional portal are closer than they appear.

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: [5802] compile problem

#25 Post by Geoff the Medio »

Bigjoe5 wrote:...The member variable I added...
That member variable seems like a bad idea if you're returning references or pointers to it. If the reference or pointer were kept by calling code, and then the iterator goes out of scope, then the reference or pointer would be invalid, wouldn't they?

User avatar
Bigjoe5
Designer and Programmer
Posts: 2058
Joined: Tue Aug 14, 2007 6:33 pm
Location: Orion

Re: [5802] compile problem

#26 Post by Bigjoe5 »

Geoff the Medio wrote:
Bigjoe5 wrote:...The member variable I added...
That member variable seems like a bad idea if you're returning references or pointers to it. If the reference or pointer were kept by calling code, and then the iterator goes out of scope, then the reference or pointer would be invalid, wouldn't they?
Yep.
Warning: Antarans in dimensional portal are closer than they appear.

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: [5802] compile problem

#27 Post by Geoff the Medio »

Is there an important reason not to use the static_cast via const void* workaround / hack then?

User avatar
Dilvish
AI Lead and Programmer Emeritus
Posts: 4768
Joined: Sat Sep 22, 2012 6:25 pm

Re: [5802] compile problem

#28 Post by Dilvish »

Bigjoe5 wrote:What's really weird is that Objects().begin<System>() isn't a standard std::map<int, T*>::iterator.
ah, I hadn't looked through Geoff's patch so carefully as to see he changed that, was still basing my thinking on the previous version where Objects().begin<System>() returned a Map<System>().begin() -- dunno if that's how my compiler was still reporting it to me or if I was just lucky that my change worked :lol:
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

User avatar
Bigjoe5
Designer and Programmer
Posts: 2058
Joined: Tue Aug 14, 2007 6:33 pm
Location: Orion

Re: [5802] compile problem

#29 Post by Bigjoe5 »

Geoff the Medio wrote:Is there an important reason not to use the static_cast via const void* workaround / hack then?
Nope, go ahead. I'll see if that iterator can be removed completely this weekend.

edit: Vezzra and Dilvish, would you mind testing that change before I commit it? I'll post a patch here when it's done.

edit2: In the meantime, I went ahead and committed the ObjectMap_misc_related.patch that Dilvish posted.

edit3: Ah, I just remembered why my approach works / why I thought it would work / why it would work, if I returned by value from operator * and changed operator -> to "return &current_value". It's because nobody in their right mind stores the result of the -> operator. :lol: I do want to get rid of that iterator entirely anyway though - it's been more trouble that it's worth, IMO.
Warning: Antarans in dimensional portal are closer than they appear.

User avatar
Dilvish
AI Lead and Programmer Emeritus
Posts: 4768
Joined: Sat Sep 22, 2012 6:25 pm

Re: [5802] compile problem

#30 Post by Dilvish »

my patch program says it's stripping extra CRs from the ends of lines for these patches, but it still rejects matching of all chunks (didn't used to see this problem with patches I downloaded here). Anyway, I'll do it manually & report back in a few minutes
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

Post Reply