Quote:
Originally Posted by
CornedBee
1) A certain crash in LookupSetRefCount(): first it deletes m_pInfo and then it calls DeletePointerInfo, which in turn accesses m_pInfo without checking (in order to delete m_pInfo->m_pdwCount) and then deletes m_pInfo again. That's an access to deleted memory and a double delete.
You're right. LookupSetRefCount() should not delete m_pInfo; DeletePointerInfo() should. Fixed.
Quote:
2) Const correctness issues. At various points you use const T*, for no good reason, and later you have to cast away the constness again.
Remember this simple rule: if you have to cast away constness for any reason other than a broken legacy API, your code is broken.
This is my thought too. It should always be const unless I have to do away with const. The only exceptions are certain functions that does not modify the pointer and does not take a const pointer. I'm going to look over the code to see if I can find any, but you can always help me point out what you found.
Quote:
3) Exception safety. If I call New() and the pointer info's CCriticalSection::Lock() throws (it can), the memory allocated by New() is leaked. Worse, if the static lock of the map fails, the local lock is never unlocked! You also leak if allocating the pointer info or the reference count fails.
I added checks to catch CMemoryException* if a Lock() fails in SetNewPointer. If it fails, newly allocated is deleted and the function fails with an ASSERT (also prints a TRACE with error info).
Quote:
4) Various subtle threading issues. Most importantly, your reference count reduction is not guaranteed to be atomic and is thus subject to concurrency failure. Nor is it included under the lock, which can be a problem under obscure circumstances.
Of course, bug. Now it should be fixed. I don't know if I should require atomic and use InterlockedIncrement/Decrement. The stupid functions require a signed LONG. Locks should be enough, or should it?
Quote:
5) Leaks due to interface circumstances that ought to be forbidden. In particular, do this: make the not time critical, point to some memory, then call SetTimeCritical(true) and destruct the pointer. Result: the map entry is leaked. (I predicted such problems, by the way.)
Initially, I used the two functions because there was no constructor to do so. Now I've disabled those functions: objects must be made thread safe or time critical through a call to the correct constructor.
Quote:
6) An overloaded interface that tries to do way too much.
The whole thing about being able to point to both arrays and single objects is a mess. It's unintuitive and brittle. I just can't see it as useful either: I can't think of a function that might accept both an array and a single object (as opposed to a 1-element array). At least not one where I would call the design sane.
Your map lookup and runtime setting of "fast mode" have already led to the problems I predicted.
I don't find the design bloated at all. It provides common access to functions you might use with pointers. What exactly do you find bad?
And the class should NEVER EVER point to a local variable. It should always point to an object on the heap. At first, I only allowed memory created through the class to be used with it, but then I realized I had to have an Attach function to attach memory pointers when I had to handle memory and the pointer came from somewhere that did not use the class.
Quote:
7) A convoluted implementation that you have yourself lost track of. Problem #1 shows this.
Meaning?
Quote:
Another, harmless incarnation of this problem is shown by the completely superfluous InternalDelete(). This little method first calls InternalDetach() and then Init(). What you forgot is that InternalDetach() either returns immediately if m_pInfo is null (which implies that p is null, too, unless you have a really big leak in that code) or calls Init() by itself. Either way, InternalDelete()'s call to Init() is redundant. And because of this, its implementation could be reduced to just calling InternalDetach(), which means it's an alias of this method and should be summarily removed.
Quite. I may look at trying to remove superflous function calls. Though they do not harm that much and being there also, they make sure that Init() is called (that the correct variables are NULL).
Quote:
A similarly superfluous method is AssignNewPointer(). In C++ tutorials, this is commonly shown as an example of a bad function:
Code:
int add(int a, int b) { return a + b; }
AssignNewPointer() is just that: a single, simple statement wrapped in a function name.
Not to mention, the main reason it took me so long to get into this code is that it is so all over the place. You seem to have a lot of redundancy in your implementation, leading to hard to understand code.
I don't remember if the function did something else before, but as you say, I'll remove it.
Quote:
8) Various minor issues.
Your Realloc() and ReallocElements() methods call realloc() on memory allocated with new. That's a big no-no. Worse, you completely misunderstand the way realloc() works and thus, in addition to invoking undefined behaviour due to memcpying non-PODs, you also leak the entire memory if realloc() has to relocate, and you're left with a dangling pointer, too.
That's not good, huh? I will have to find another memory re-allocation function if so, but you can always throw me suggestions.
Quote:
Operator + is indeed dangerous and best removed.
Done.
Quote:
Allocating a new, special object just to assign null to a pointer? WTF??? Not to mention that people might be tempted to compare pointers against PPNULL (It makes sense. After p = PPNULL; it should be expected that p == PPNULL. Not only is the comparison false, it also leaks memory.)
Well, I think initially because of compiler's habit of casting any type to int. I can't seem to fix it either. Making an int constructor explicit only cuses compile errors when I try to return NULL from a function that return a CMemoryManager object.
Quote:
Why is the reference count not directly embedded in the PointerInfo struct? Why allocate it separately? Doing that just exposes you to problems.
Ah, of course, my mistake. It was a pointer before, because it wasn't attached in a struct at all. I got the idea to actually keep more information about each pointer, so I put them into a struct.
Quote:
What's this TempPointerInfo? Why the name? An hour has passed now since I started looking at your post, and still I don't really have an idea.
It's used for the map and caching. It puts the actual PointerInfo pointer (for the current memory) in there, along with the pointer (memory pointer, p) it's attached to and if I've looked up the current p in the map yet.
Basically, the flow should be:
operator = called with a new address.
ArrayIsMemory() is called and check if the the pointer passed has been looked up in the map yet (like if you assign the class the same pointer twice, it won't look up the pointer info in the map again) and if it hasn't, it does. Then it fills the struct that it has looked up p and the PointerInfo* for p.
When LookupSetRefCount() is called, it does a similar check and will find that the information has already been looked up (because m_PTempInfo.bLookedUp == true && m_PInfo.p == p) so it doesn't need to look it up in the map again but goes ahead and replaces the current m_PointerInfo with the one found in the map.
I've edited the first post with the updated code.
Also I seem to have problems declaring operator = and operator == explicit :/ I'm not sure that means the compiler can make stupid code again and call the operator with a non-int.
Quote:
Originally Posted by
brewbuck
I'm not saying it looks poor. I'm just not sure what you plan on doing with it.
Of course, to manage my obsessity with pointers and objects on the heap. I'm already using the class, of course.
Quote:
Originally Posted by
cpjust
Well I don't really have time to read the whole thing (plus my brain is melting from looking at really bad C code all day), but if you're trying to create something like an auto_ptr class, I wrote my own a few years ago. You can check how it works and maybe get some ideas (I basically stole, err, I mean borrowed most of it from the STL auto_ptr class):
http://tech.groups.yahoo.com/group/boost/files/AutoPtr/
Oh crap. Sign in to Yahoo >_< No thanks. No offense to you, just to Yahoo.