Ranged Based Iteration Syntax for Linux

[Lokked]

Hi Blackstorm! (this is for everyone, but Blackstorm is our resident Linux compile expert!)

Because you compile exclusively on Linux, are you able to determine why we cannot do this with GCC, but we can with MSVC?

Code: Select all

for (auto &itr : some_STL_container) {

This one isn't actually such a big deal, because we CAN do this:

Code: Select all

for (auto const &itr : some_STL_container) {

And the normal way to iterate through lists (not range based, with the Colon), I noticed you remove the Ampersand in some, so I'm assuming it wouldn't work:

Code: Select all

 for (auto &itr = some_STL_contrainer.begin(); itr != some_STL_contrainer.end(); ++itr) {

We use the "old fashioned way" in case we need to modify the container, so we wouldn't be able to use a const qualifier.

There's got to be a way to use the reference! We just need to figure it out. This is more of an issue because we use shared_ptr<>. When you iterate through an STL container with a non-referenced itr, it creates a copy of the object to do so, which wastes processing power for copying the object and with shared_ptr<> it does a reference count ++ and then a reference count -- once we're done with that loop.

From McRae:
"If you want to modify the variable (or avoid passing around large data types by value) and the underlying iterator supports it, you should make the loop variable a reference:"

I found a reference for something that may be of value. NOTE: This applied to Range Based For Loops only! (syntax: for (auto itr : some_STL_contrainer) {)
http://en.cppreference.com/w/cpp/language/range-for
Example near the bottom.

For passing by reference, and thus avoiding the copying, use 2 Ampersands (&&).

Blackstorm, can you try this in one of the range based for loops and see if it compiles? If it does, I'll try updating a few and commit and we'll see if it crashes or not.

[Lokked]

Well now I wonder how this works with Linux (it's been here since the beginning):

Code: Select all

shared_ptr<Client> UDPServer::GetClient(uint32_t account_id) {
    shared_ptr<Client> client;

    m_clients.ReadLock();
    for (auto& itr : clients) {  // WTF??
        if (itr.second->GetAccountID() == account_id && itr.second->IsConnected()) {
            client = itr.second;
            break;
        }
    }
    m_clients.ReadUnlock();

    return client;
}

[zippyzee]

From what I have seen of the edits, this works fine:

Code: Select all

for (auto &itr : char_inventory)

But this does not:

Code: Select all

for (auto &itr = char_items.begin(); itr != char_items.end(); itr++)

and needs to be:

Code: Select all

for (auto itr = char_items.begin(); itr != char_items.end(); itr++)

In this case char_inventory is called within worldcharacter.cpp, and is defined in worldcharacter.h as a map with pointers to item structs based on unique id's. char_items is the same thing, but in chunkserver.cpp, and references char_inventory through character->GetInventory().

[Blackstorm]

Hi^^

Thanks for the title Looked ^^ but really i am a noob in C++, i think others devs should have a better answer for you like Zippyzee demonstrate ^^

I just know, the "&" before, always kill/break the gcc compiler in this context. so for this example i had replaced the value's type without the "&" and it's work fine. (like we done for each of these cases several months ago or with add a 'const')

I don't have always the better solution for the context but its compile correctly on gcc.

Just always remember, the value i have replaced give a good context for the compiler but not always for the code. Each time i edit the dev file for a recent function (added or modified) that break the compilation or if some errors of comparison are made, the functions should be rechecked to make sense (it's why you ask here ^^ so i presume you have a better way ^^)

"Because you compile exclusively on Linux, are you able to determine why we cannot do this with GCC, but we can with MSVC?"
-> Because MS Tools are less strict than Linux tools, and follow the "MS" best practices and not the "world" best practices ^^, maybe.. maybe not

Otherwise, yes, i will check your solution this afternoon, if we can improve the solution for that specific compiler issue (like you done with the memcpy vs strncpy ^^) it's always better.

NB: actually the Makefile have a specific C++0x and not C++11 flag, so maybe now we can level up that flag ...? or why not ? (MSVC you are using should be flaged with c++14)

In other hand, i will check with John if he agree to have an appliance based on a VM with all tools pre-installed on a Ubuntu/Debian, if some devs are interested to check themselves the compilation under that distrib), i can make a general "How To" with all the tools you need to interact with a Windows client on the forum private side...

Edit1: Perfect, the rev841 is a very good revision to try it, my first compilation error is :

Code: Select all

WorldDatabase.cpp: In member function 'bool WorldDatabase::CombineNPCs(std::vector<unsigned int>&)':
WorldDatabase.cpp:200:45: error: invalid initialization of non-const reference of type 'std::_Rb_tree_iterator<std::pair<const unsigned int, unsigned int> >&' from an rvalue of type 'std::map<unsigned int, unsigned int>::iterator {aka std::_Rb_tree_iterator<std::pair<const unsigned int, unsigned int> >}'
  for (auto &itr = spawn_id_chance_map.begin(); itr != spawn_id_chance_map.end(); ++itr) {
                                             ^
Makefile:138: recipe for target 'WorldDatabase.o' failed
make: *** [WorldDatabase.o] Error 1

so i will do my tests with this rev.

edit2 :
Original (not working) :

Code: Select all

for (auto &itr = spawn_id_chance_map.begin(); itr != spawn_id_chance_map.end(); ++itr) {

Not working :

Code: Select all

for (auto& itr = spawn_id_chance_map.begin(); itr != spawn_id_chance_map.end(); ++itr) {

Working :

Code: Select all

for (auto &&itr = spawn_id_chance_map.begin(); itr != spawn_id_chance_map.end(); ++itr) {

or

Code: Select all

for (auto itr = spawn_id_chance_map.begin(); itr != spawn_id_chance_map.end(); ++itr) {

i tried to use the "&&" syntax and its compiling on rev841:
Changed from rev841 to rev842 (only for that error) :
WorldDatabase.cpp:200
Commands.cpp:1726
Commands.cpp:1751
Commands.cpp:1761
Commands.cpp:1842
Commands.cpp:1902

[Lokked]

Blackstorm, can you take one example that was changed to remove the & symbol and make it a && and see if that works?

An easy example I recall that didn't compile was in combat.cpp. In the ProcessCombat() function (might be just Process()), the first thing that happens is a list is checked for being empty and then there is a line something like:

Code: Select all

for (auto itr : combat_list) { //Might not be combat_list. I can't remember. It's close to top of ProcessCombat function.

and add the && before itr.

Let me know if this works or not. The flag set on the makefile is interesting. Range Based For Loops weren't even a thing in c++0x. I thought they were first introduced in 11. I might be wrong. See if changing the flag works.

AI and Combat Loops will be where performance is necessary. If and when we switch to raw pointers, it won't make a difference at all, but for now, with reference counted pointers, the overhead will add up.

[Blackstorm]

done with my previous post " i have edited it^^" -> come to the IRC if possible and if needed ^^

I am checking your specific ask about combat.cpp

Edit: Ok so if i add the double "&" to the 2 iterations of "(auto itr" on line 85 and 133 i've found into the Combat.cpp file, it's working.

NB: Changing the flag by : CXXFLAGS=-std=c++11 doesn't resolve the problem

[Lokked]

Ok, great!. Let's keep things WITHOUT the && for now. Let's keep this thread as is and use it as a reference. At some point, I want to benchmark using with and without the && to see if it's getting us anywhere in terms of performance. The internet tells me it won't copy the underlying object (or pointer), but without profiling it's tough to tell if we're gaining anything.

[John Adams]

FYI: my edits removing & were on the advice of Scatman; purist ANSI-C dude. While I would follow him into the fires of Hell, you guys do whatever you need to do and standardize it from here out. This means, fix old code that doesn't comply when we change styles. Please.