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.

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.

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.

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.

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.)

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.

7) A convoluted implementation that you have yourself lost track of. Problem #1 shows this. 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.
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.

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.
Operator + is indeed dangerous and best removed.
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.)
Why is the reference count not directly embedded in the PointerInfo struct? Why allocate it separately? Doing that just exposes you to problems.
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.

Whoa, an hour! I certainly spent a lot of time on this. But after the way I keep criticizing your ideas, it seemed fair that I actually look at what you produced.

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/

There's also the cases where the compiler converts and uses faulty code in the smart pointer. Having a constructor that takes a single bool, you could actually assign an integer to the class and the constructor will construct a temporary class and call the bool constructor and then assigning it, which is plainly wrong. I want to avoid such mishaps, as well.

That's exactly what the explicit keyword is for.
If you look at my AutoPtr constructors, you'll see that I declared them as explicit to prevent automatic type conversions.

It may look poor to you because I'm pretty much self-learned
Mostly I haven't tested much of the new implemtations, though so far it has worked.
But as I did mention, I'm here to learn
I'll have a look at those comments and change the class accordingly.

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.

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.

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).

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?

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.

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.

7) A convoluted implementation that you have yourself lost track of. Problem #1 shows this.

Meaning?

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).

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.

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.

Operator + is indeed dangerous and best removed.

Done.

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.

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.

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.

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.

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.

This is my thought too. It should always be const unless I have to do away with const.

No, you misunderstand me. It should never be const. The class deals with a pointer to T. Not with a pointer to const T. If the user wants a pointer-to-const, he can do CMemoryManager<const CFoo>.

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?

Locks are enough, provided the compiler recognizes them as memory barriers. (I'm pretty sure it does.) Of course, making the refcount a LONG wouldn't hurt either, seeing how you're unlikely to ever get even close to two billion references to an object.

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?

It tries to make decisions that should be compile-time issues runtime issues. That makes it more fragile and more overloaded. You should have different smart pointer classes for different purposes. You should have one smart pointer for array allocations (better yet, you should use a standard container for arrays) and one for single object allocations. (Boost has shared_ptr and shared_array.) You should have one smart pointer that is thread-safe and one that is not. This way, at least you can easily see from just looking at the declaration whether you're allowed to pass this pointer to another thread. You don't even have to look at the constructor call. (Boost doesn't have a thread-unsafe smart pointer. It's considered superfluous. Boost's shared_ptr uses lock-free reference counting and is quite fast.) The whole performance-sensitive thing is nonsense - in performance-critical situations, you don't use a reference-counted smart pointer at all. You have tight inner loops that use references to objects. And beside the absurd pointer recapture facility you have, the only thing your performance switch does is disable some checks that are only done in debug mode anyway. Get rid of the machinery, use the smart pointer only where you really want to keep a hold on it, and you'll find that the whole issue goes away.

And the class should NEVER EVER point to a local variable.

Of course it shouldn't. The whole point (pun not intended) of a smart pointer is that it manages memory.

Meaning?

Meaning you lost track of which function is responsible for deleting m_pInfo. That really shouldn't happen. That wouldn't happen if the implementation wasn't so complicated and all over the place.

That's not good, huh? I will have to find another memory re-allocation function if so, but you can always throw me suggestions.

There isn't one. There's allocating a new block and copy-constructing the objects over. That's what std::vector does.

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.

And you can't do that in local variables and function parameters, why? Note that you never need this data after initialization, yet your class carries (taking likely alignment issues into account) 3*sizeof(void*) bytes (that's 12 on 32-bit systems, 24 on 64-bit systems) around all its lifetime, even duly copies the zeros over on copy construction. (Or does it? Or does it leave those values uninitialized?)

Basically, the flow should be:
operator = called with a new address.
ArrayIsMemory() is called and checks if the the pointer passed has been looked up in the map yet and if it hasn't, it does.

Wonderfully convoluted. It's completely non-obvious that a function called ArrayIsMemory() does the lookup. You should simply do the lookup and have it set class members, and only if it fails do anything else.

When LookupSetRefCount() is called, it does a similar check and will find that the information has already been looked up

Alarm bells should be ringing in your head when you write stuff like that. There obviously is a lack of clearly defined responsibilities here.

Also I seem to have problems declaring operator = and operator == explicit

You can't. Only constructors can be explicit.

I'm not sure that means the compiler can make stupid code again and call the operator with a non-int.

It can't use the functions for implicit conversions. It can still implicitly convert the arguments from something else, do something you didn't quite expect, compile code you didn't expect to compile.

No, you misunderstand me. It should never be const. The class deals with a pointer to T. Not with a pointer to const T. If the user wants a pointer-to-const, he can do CMemoryManager<const CFoo>.

Then it's impossible to send the class through a const reference since it can only call const functions.

Locks are enough, provided the compiler recognizes them as memory barriers. (I'm pretty sure it does.) Of course, making the refcount a LONG wouldn't hurt either, seeing how you're unlikely to ever get even close to two billion references to an object.

Shrug. You never know.

It tries to make decisions that should be compile-time issues runtime issues. That makes it more fragile and more overloaded. You should have different smart pointer classes for different purposes.

I'm not going to have 1000 classes that does the same thing. Splitting the class would be difficult or very tiresome as I'd have to rewrite a lot of code (since calling Release calls InternalDelete which calls InternalDetach, which Locks, deletes memory and removes pointer from map).
Ah, unless I make virtual functions and override those with non-locking and non-pointer looking-uping.
Maybe later.

The whole performance-sensitive thing is nonsense - in performance-critical situations, you don't use a reference-counted smart pointer at all. You have tight inner loops that use references to objects. And beside the absurd pointer recapture facility you have, the only thing your performance switch does is disable some checks that are only done in debug mode anyway. Get rid of the machinery, use the smart pointer only where you really want to keep a hold on it, and you'll find that the whole issue goes away.

Not nonsense in my view. Try assigning a lot of pointers to the class a lot of the time and you'll see a performance hit. But if you still want to use the class with heavy memory usage, you can easily use the performance critical type.

Meaning you lost track of which function is responsible for deleting m_pInfo. That really shouldn't happen. That wouldn't happen if the implementation wasn't so complicated and all over the place.

No, that was a bug. Initially, I just wanted to delete m_pInfo, but later realized that I had to delete m_pInfo->pdwRefCount, so I made a new function to handle that and called it but forgot to remove delete m_pInfo.

There isn't one. There's allocating a new block and copy-constructing the objects over. That's what std::vector does.

That's really stupid. I don't see why I can't use realloc, though. Even though you disagree, it can work, as I have used it in the past and is has worked fine. I don't know the implications of doing it, however.

And you can't do that in local variables and function parameters, why? Note that you never need this data after initialization, yet your class carries (taking likely alignment issues into account) 3*sizeof(void*) bytes (that's 12 on 32-bit systems, 24 on 64-bit systems) around all its lifetime, even duly copies the zeros over on copy construction. (Or does it? Or does it leave those values uninitialized?)

It simply doesn't work that way. Instead of looking up twice (I agree that could be done with arguments), but caching wouldn't work that way. Though I could remove the caching. I don't know if this would be a good idea, but I don't think it will have much of a performance impact.

Wonderfully convoluted. It's completely non-obvious that a function called ArrayIsMemory() does the lookup. You should simply do the lookup and have it set class members, and only if it fails do anything else.

Well, maybe I should do a separate Lookup() function that looks it up (if it hasn't been already) and when it returns (unless an exception is thrown), the function that called Lookup can check the m_PInfoTemp var for info it needs.

You can't. Only constructors can be explicit.
It can't use the functions for implicit conversions. It can still implicitly convert the arguments from something else, do something you didn't quite expect, compile code you didn't expect to compile.

Now that is a shame, because I don't want the compiler to do all sorts of weird conversions before calling those operators.
And now I have to post the newest code since I can't edit the first post:
What's mostly new is Resize with realloc works as it should now (or so I think) and exception casting if something goes wrong. It also implememts a stack system, so it's possible to see the functions, in order, after the exception was thrown. Dunno how much use it has, but it seemed cool to me.

Now that is a shame, because I don't want the compiler to do all sorts of weird conversions before calling those operators.

That's just a fact of life when dealing with integer-like types (including bool). You can wrap fundamental types in simple wrappers which will prevent automatic conversions, but you have to make their constructors explicit or you get all the same problems again. And explicit constructors means you have to do something silly like:

That's just a fact of life when dealing with integer-like types (including bool). You can wrap fundamental types in simple wrappers which will prevent automatic conversions, but you have to make their constructors explicit or you get all the same problems again. And explicit constructors means you have to do something silly like:

Code:

myFuncWhichTakesBool(MyBool(true));

Instead of just:

Code:

myFuncWhichTakesBool(true);

Now that's something that should make it into the standard: no weird automatic conversions. If a function takes a bool, then it takes a bool, end of story. Not some int or any other kind of data type that the compiler can think of making up.

That's really stupid. I don't see why I can't use realloc, though. Even though you disagree, it can work, as I have used it in the past and is has worked fine. I don't know the implications of doing it, however.

For one thing consider a class that needs to notify some other classes if its address changes. And I'm not sure if you are even guaranteed to be able to delete memory if the pointer has gone through realloc.

Anyway, how comes a smart pointer is moving objects around? Shouldn't it manage pointers? (May-be leave the first task to dedicated classes such as std::vector?)

Now that's something that should make it into the standard: no weird automatic conversions. If a function takes a bool, then it takes a bool, end of story. Not some int or any other kind of data type that the compiler can think of making up.

I don't see what's the problem with that. You are writing classes for programmers (including yourself) and not idiots. It's the users problem if they don't bother to look up references before starting to pass random and inappropriate values...

The users will always find a way to misuse your class. For example what happens if I delete the pointer that I have just given to a smart pointer?

By the way, MSVC has a performance warning: forcing integer into bool (or something like that). I suppose to get rid of it, I'd have to be more explicit (although I'm not sure how that would improve performance):

Anyway, how comes a smart pointer is moving objects around? Shouldn't it manage pointers? (May-be leave the first task to dedicated classes such as std::vector?)

I don't get what you mean?

I don't see what's the problem with that. You are writing classes for programmers (including yourself) and not idiots. It's the users problem if they don't bother to look up references before starting to pass random and inappropriate values...

The users will always find a way to misuse your class. For example what happens if I delete the pointer that I have just given to a smart pointer?

It helps catch bugs at design time and not create weird bugs in your code when the compiler calls the entirely wrong function. I was frustrated before when I patched

Code:

myclass = 50;

...to the bool constructor that took a param that said if you wanted to create a new object when creating the class or not!
It should have failed by then (code has since been revised).