Some cleanups in Bag.cpp/Bag.h

[Toinan67]

• * What bug does the patch fix? What features does the patch add?
  Fixes nothing, adds some cleanup to the code itself
  -Remove redundant empty line
  -Code style cleanup
  -Remove useless includes
  -Use a vector for bag slots, instead of a simple array ("more" C++)
  -Use of uint8 instead of uint32
  -Add comments for each function
  Do not laugh if this patch is completely invalid. Please be aware that I'm not sure of myself at all, I'm here to get your comments (even if the patch is nearly nothing...)
  * For which repository revision was the patch created?
  10288+
  * Is there a thread in the bug report section or at lighthouse? If yes, please add a link to the thread.
  No
  * Who has been writing this patch? Please include either forum user names or email addresses.
  My fingers
  

The patch -> http://paste2.org/p/936165

[Schmoozerd]

-    for(int i = 0; i < MAX_BAG_SIZE; ++i)
-        if (m_bagslot[i])
-            delete m_bagslot[i];
+       m_bagslot.clear();

the clear of a vector just flushes the elements, not deletes them, so this might be a mem leak here

either iterate the the m_bagslot or look if there is a for_each operator in C++ available

And if I am not mistaken, there is no need to check a pointer before delete.

[Toinan67]

That's what I thought too, but see here : http://www.cplusplus.com/reference/stl/vector/clear/

"All the elements of the vector are dropped: their destructors are called, and then they are removed from the vector container, leaving the container with a size of 0."

Who's right?

And if I am not mistaken, there is no need to check a pointer before delete.

I don't know, maybe it is needed because "delete m_bagslot" calls the ~Bag function of the object. If the pointer is null, you'll have a crash when calling ~Bag.

Am I right?

[Lynx3d]

And if I am not mistaken, there is no need to check a pointer before delete.

Yes, delete checks for Null-pointers, yet many many people still do it redundantly, probably because they learned C before where free() did NOT check pointers.

I had seen it that often that i even believed myself it was needed when i started C++, but it isn't.

Who's right?

Both

clear() calls the destructor of the element type. Elements here is Item* (pointer to Item), pointers do not have any real destructor, they only hold a memory address.

So yes, you do leak the memory from all items if those were supposed to get deallocated here.

[Lynx3d]

Hm a few more exentsive comments:

-Use a vector for bag slots, instead of a simple array ("more" C++)

Debatable if it's really worth it in this case, just adds additional indirections for marginal potential memory savings...and you still use MAX_BAG_SIZE in any case instead of actual bag size, so no win here.

-Use of uint8 instead of uint32

Another myth from god knows what times of early computer history is that "samller int types are faster", with current CPUs you can at worst even hurt performance when not using native word size (32 or even 64bit these days) because memory subsystem is optimized for memory alignment in multiple of its native word size. Also, for examble GetBagSize() returns uint32 anyway, so your uint8 is widened anyway to compare it to the return value.

-Add comments for each function

If i'm not mistaken Vladimir said trivial documentation such as

"@param slot The slot where the item is"

"@param limitedCategory The limitedCategory to look for"

is not desireable, function and argument names shall make such documentation needless, that's what is called self-documenting code.

So only add additional documentation to things that actually need it.

[Toinan67]

Thanks for your posts Lynx3d

I realize I should have posted in core modifications

Debatable if it's really worth it in this case, just adds additional indirections for marginal potential memory savings...and you still use MAX_BAG_SIZE in any case instead of actual bag size, so no win here.

I've always been told to use vectors, maps, lists, etc in C++, because "it's better". Even when a simple array would be enough. Bad habit? When you don't need a lot from a vector, better to use simple arrays?

For uint8 vs uint32, actually I was thinking as with a database (smallint better than int when you can for example). This "myth" comes from database for me. Thanks for the tip.

And about documentation, it's a bit the same than vector, I've always been told not to hesitate adding "useless" documentation, because for some people it is not useless (particularly in an educational project)

[VladimirMangos]

about vector/array... if you have fixed elements amount and need always existed storage then array can be better.

[Toinan67]

...and that's what we have with the bag slots : an array who has a fixed size MAX_BAG_SIZE : no need to change the actual code.

clear() calls the destructor of the element type. Elements here is Item* (pointer to Item), pointers do not have any real destructor, they only hold a memory address.

So yes, you do leak the memory from all items if those were supposed to get deallocated here.

I find it quite confusing. If I understood well, clear() does not do something like

for (int i=0; i < myVector.size(); ++i)
   delete myVector[i];

But just calls the destructor of myVector, so something like this (simplified):

for (int i=0; i < myVector.size(); ++i)
   myVector[i]->~ElementType();

I didn't know it was possible actually. Just found it on google, "C++ destructor explicit call"

If I'm allowed to "defend" the vector : it does the "memset" part itself, which is good, right?

Edit : and another question (sorry if I ask too much x) ) for uint8...why does MaNGOS uses it? Does it have any advantage?

[VladimirMangos]

uint8 mostly used in placers where original value stored in uint8 field, in packet from client, or send to client as uint8 field, sometime if it good pack with other small fields in less size structure...

[darkstalker]

what about using std::tr1::array?

[VladimirMangos]

When you plan code changes you must ask:

1) what problems with current code? Array for example.

2) what will be improved in code/in performence/in error catching/later code support if change in planned way...

3) what losses will be from code changes (less clean code, performence losses, hard error catching, more hard support )

(2) must be > (1) + (3)