Thread: Ban pointers or references on classes?

  1. #61
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    Yes, but it illustrates a pretty simple yet devastating problem. Normally, the destructor would be called for the object and decrement the reference count, but if you pass by reference it isn't, so calling Delete() then is a problem.
    I can't think of a way to add security checks to make sure it doesn't happen.
    It may be a programmer screwup. Delete() is safe to call on a smart pointer, because it only decrements the ref counter and as long as other objects exist, they are gauranteed to have access to the memory during their lifetime.
    Don't we often set pointers to NULL ourselves, when we don't need them, maybe to avoid programming errors? There are also other synonyms, other functions for the same: Erase and RemoveRef. Both call Delete.
    This shows how a simple programming error, that should be safe to do with raw pointers and a smart pointer class passed by value, becomes devastating when passing the class by reference.
    I might want to do something about that. More than putting a big, fat warning, of course.

  2. #62
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,895
    Quote Originally Posted by Elysia View Post
    Yes, but it illustrates a pretty simple yet devastating problem.
    The devastating problem here is that the function takes a non-const reference to the smart pointer when it has no business modifying it. In addition, because you are hell-bent on demonstrating the "danger" of references, you neglected to make main(), which knows it's passing the pointer by non-const reference and should thus expect modifications, check for said modifications after the call returns. There may be a legitimate use case for the called function to call Delete(). If there is, have main() check the pointer for null after foo() returns. If there isn't, make that reference const.

    I can't think of a way to add security checks to make sure it doesn't happen.
    That's fine. The rule is: protect against Murphy, not Machiavelli. Make sure innocent mistakes are detected. You can't do anything about programmers intentionally screwing things up.

    It may be a programmer screwup.
    Let's see. This programmer takes the smart pointer by reference, clearly intending to modify it, proves this intention by calling a function that modifies it, but neglects to account for the modification in the calling function.
    Nope, doesn't sound like an honest mistake.

    Don't we often set pointers to NULL ourselves, when we don't need them, maybe to avoid programming errors?
    Umm ... no. Pointers are set to NULL after the object they point to is deleted. But that's for raw pointers. Smart pointers are mostly left until they go out of scope. And in particular, you don't do such a thing with a borrowed smart pointer. Such a mistake could only happen to someone who has this idea very, very deeply buried in their mind. Normal programmers wouldn't do it.

    There are also other synonyms, other functions for the same: Erase and RemoveRef. Both call Delete.
    By the way, both Delete and Erase are rather poor names. They are misleading. You're neither deleting nor erasing the object pointed to (unless the reference count drops to zero). Nor are you deleting or erasing the pointer itself, because it probably hasn't been allocated on the heap, and if it had been, it still wouldn't be what the function does. RemoveRef is somewhat acceptable, but sounds very much like it decreases the reference count without any further modification. Good names are Release or Clear, or the standard (as in, the C++09 smart pointers use it) Reset. And you should choose one name and stick with it, as having synonyms for the same functionality is generally not a good thing. It makes the interface larger without any extra gain. (basic_string::length has been widely criticized for being superfluous. It's a historical remainder, kept for backward compatibility, really.)

    This shows how a simple programming error, that should be safe to do with raw pointers
    Absolutely not! You're comparing apples and oranges here, because raw pointers don't have a reference count. If you think that calling Delete() is just equivalent to setting the pointer to NULL, well, the problem is that you've been passed the pointer by reference, thus any modification is visible outside. Which brings us back to the question of why you pass the pointer by non-const reference in the first place.

    and a smart pointer class passed by value
    If you want a modifiable copy, make a modifiable copy. I don't see what you need it for, though.

    becomes devastating when passing the class by reference.
    You're not taking the no modifications contract the by-value or by-const-reference calling conventions have, and then you complain about what happens when modifications occur. Well, duh! It's just another straw man attack.

    I might want to do something about that. More than putting a big, fat warning, of course.
    I recommend that you work with people who aren't out to get you.


    On a vaguely related note, foo() should take an int& in the first place, and you should dereference the pointer on calling foo(). Unless foo() for some reason needs to keep the pointer in a place that outlives the function call.
    Last edited by CornedBee; 10-29-2007 at 04:31 AM.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  3. #63
    and the hat of sweating
    Join Date
    Aug 2007
    Location
    Toronto, ON
    Posts
    3,545
    Umm ... no. Pointers are set to NULL after the object they point to is deleted. But that's for raw pointers.
    Maybe I'm just not reading this statement correctly, but if you're saying that a pointer is automatically set to NULL once you call delete on it, then that's wrong.

    As for all these examples of references being dangerous... Rather than going to hell and back to try to ban references, wouldn't it be easier to design all your functions to either pass pointers or by value? Personally, I think pointers are a lot more dangerous than references, but that should solve this problem right?

  4. #64
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    Maybe I'm just not reading this statement correctly, but if you're saying that a pointer is automatically set to NULL once you call delete on it, then that's wrong.
    I think you are reading an "automatically" into CornedBee's words when such an effect was neither stated nor intended.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  5. #65
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,895
    Indeed. You should read a "by convention" into the statement, not an "automatically".
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  6. #66
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    Let's just make one thing straight here. I'm not saying references are dangerous and I want to ban them anymore. I'm just saying that passing by reference may expose the code to big errors from simple mistakes that simply doesn't exist when you pass by value. Programming errors. Programming mistakes.

    Quote Originally Posted by CornedBee View Post
    The devastating problem here is that the function takes a non-const reference to the smart pointer when it has no business modifying it.
    OK well, a problem lies in that with pointers, you usually pass them by value. Then you keep on modifying them all you want.
    Though I just tried (not to keen on const), but a const reference would mean you can't call non-const functions. That would mean it's impossible to call Delete(). That's good.

    In addition, because you are hell-bent on demonstrating the "danger" of references, you neglected to make main(), which knows it's passing the pointer by non-const reference and should thus expect modifications, check for said modifications after the call returns. There may be a legitimate use case for the called function to call Delete(). If there is, have main() check the pointer for null after foo() returns. If there isn't, make that reference const.
    Even if it checks, the memory is gone. Defeats the purpose of smart pointers.

    Umm ... no. Pointers are set to NULL after the object they point to is deleted. But that's for raw pointers. Smart pointers are mostly left until they go out of scope. And in particular, you don't do such a thing with a borrowed smart pointer. Such a mistake could only happen to someone who has this idea very, very deeply buried in their mind. Normal programmers wouldn't do it.
    Just setting a pointer to NULL (and NOT a passed by reference pointer) is good programming practice when you don't need to use it anymore. Situations can vary, but for example, you are not supposed to free the memory and no longer have a use for it. Set to NULL.

    By the way, both Delete and Erase are rather poor names. They are misleading. You're neither deleting nor erasing the object pointed to (unless the reference count drops to zero). Nor are you deleting or erasing the pointer itself, because it probably hasn't been allocated on the heap, and if it had been, it still wouldn't be what the function does. RemoveRef is somewhat acceptable, but sounds very much like it decreases the reference count without any further modification. Good names are Release or Clear, or the standard (as in, the C++09 smart pointers use it) Reset.
    Those names do not make much sense to me. Delete is also tje opposite of the function New, which create a new instance of T[num] and stores it in the smart pointer class.

    And you should choose one name and stick with it, as having synonyms for the same functionality is generally not a good thing. It makes the interface larger without any extra gain. (basic_string::length has been widely criticized for being superfluous. It's a historical remainder, kept for backward compatibility, really.)
    As long as multiple functions make the code more readable, I'm all for it. It makes no sense to Delete a pointer in a linked list. But Removing the reference does.

    Absolutely not! You're comparing apples and oranges here, because raw pointers don't have a reference count. If you think that calling Delete() is just equivalent to setting the pointer to NULL, well, the problem is that you've been passed the pointer by reference, thus any modification is visible outside. Which brings us back to the question of why you pass the pointer by non-const reference in the first place.
    Well, it should imitate that behaviour by setting its pointer to NULL. Though it also decrements the ref counter, if passed by value (as was my intended design), it does not pose a problem.

    On a vaguely related note, foo() should take an int& in the first place, and you should dereference the pointer on calling foo(). Unless foo() for some reason needs to keep the pointer in a place that outlives the function call.
    Do you do that with C++ smart pointers as well?

  7. #67
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,895
    Do what? Dereference them and pass a reference to the object to functions? Of course! If a function doesn't need the information that the object passed to it is ultimately managed by a smart pointer, by all means hide that information. The less a function knows, the better. The less a function can do, the better. Give every component of your code the absolute minimum information and capabilities it needs to do what it should. Everything beyond that makes the program harder to understand. That's the rule that ultimately leads to such things as discouraging global variables, using access control for encapsulation in classes, keeping your code const-correct and so on.

    As long as multiple functions make the code more readable, I'm all for it. It makes no sense to Delete a pointer in a linked list. But Removing the reference does.
    It makes no sense to call Delete on any smart pointer if you don't have the guarantee that the pointee is actually deleted. With a reference-counting pointer, that's the general case. Therefore, it shouldn't have a method called Delete.

    Delete is also tje opposite of the function New, which create a new instance of T[num] and stores it in the smart pointer class.
    No, it's not. New is guaranteed (unless an error occurs) to allocate memory and make the pointer point to it. If Delete were the opposite, it would guarantee to free that memory and make the pointer point to nothing. But because the pointer does reference counting, Delete only guarantees the latter. Thus, it's not an opposite of New and shouldn't be called Delete. The same goes for Erase. Erase is more ambiguous in its meaning, but it's also non-idiomatic for memory handling. The name is used in the standard containers for the deletion of individual elements. RemoveRef makes sense in the context of the reference-counted smart pointer, but it leaks the idea of reference counting into the name of a method that could well exist without it. This means that a smart pointer using a different memory management method (say, a scoped_ptr or a unique_ptr) cannot have the same name for the same functionality without being misleading, and that means that the various smart pointers have different names for the same functionality, leading to a steeper learning curve for each one. Consistency in interfaces is important.
    Among my names, Clear is my least favourite. It's taken again from the containers, where it means freeing everything, completely emptying the container. Not the best association, but it still indicates a disassociation of the object from whatever is behind it.
    This disassociation is the primary focus of the name Release. It releases the pointer it has a hold on back its origin. It disassociates from the pointer. There's now one owner less of the memory. The two big disadvantages of the name come from its association. First, the auto_ptr class uses release() for real disassociation, i.e. it passes control over the pointer lifetime back to the programmer. That's not what happens for a reference-counted pointer, because the pointer cannot guarantee that no other pointer exists. Second, Microsoft's COM (and by copying, Mozilla's XPCOM) specifies that every object has a method called Release, but due to the internally reference-counted nature of COM objects, there is no nulling out pointers, and definitely no safety.
    So we're left with Reset. Reset returns to the pointer nature of the smart pointer, simply meaning "return the pointer to its original state", that state being null. That's all that Reset means: set the pointer to null. If the pointer happens to manage the memory of the pointee in such a way that the pointee is deleted as a consequence, that's fine. I like this name most of all.

    Even if it checks, the memory is gone. Defeats the purpose of smart pointers.
    No, it doesn't. The purpose of smart pointers is to impose an automated memory handling policy. What exactly this policy does depends on the smart pointer.
    Your reference-counted smart pointer has the policy, "As long as at least one pointer to a memory block exists, the memory block is kept alive."
    If main() calls foo() and passes its sole pointer object by non-const reference, it's saying, "Here's my pointer, along with whatever it points to and the power to modify what it points to." If main() needs a guarantee that the memory is still alive after the call, it shouldn't have passed its only pointer to that memory by non-const reference. It should have created a copy of the pointer first.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  8. #68
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    Quote Originally Posted by CornedBee View Post
    Do what? Dereference them and pass a reference to the object to functions? Of course! If a function doesn't need the information that the object passed to it is ultimately managed by a smart pointer, by all means hide that information. The less a function knows, the better. The less a function can do, the better. Give every component of your code the absolute minimum information and capabilities it needs to do what it should. Everything beyond that makes the program harder to understand. That's the rule that ultimately leads to such things as discouraging global variables, using access control for encapsulation in classes, keeping your code const-correct and so on.
    The only problem (or the biggest anyway) I have with that is that you're leaving the function wide open to free the memory and crashing the class. Unless
    Code:
    delete &var;
    ...doesn't work? I've never tried.

    It makes no sense to call Delete on any smart pointer if you don't have the guarantee that the pointee is actually deleted. With a reference-counting pointer, that's the general case. Therefore, it shouldn't have a method called Delete.


    No, it's not. New is guaranteed (unless an error occurs) to allocate memory and make the pointer point to it. If Delete were the opposite, it would guarantee to free that memory and make the pointer point to nothing. But because the pointer does reference counting, Delete only guarantees the latter. Thus, it's not an opposite of New and shouldn't be called Delete. The same goes for Erase. Erase is more ambiguous in its meaning, but it's also non-idiomatic for memory handling. The name is used in the standard containers for the deletion of individual elements. RemoveRef makes sense in the context of the reference-counted smart pointer, but it leaks the idea of reference counting into the name of a method that could well exist without it. This means that a smart pointer using a different memory management method (say, a scoped_ptr or a unique_ptr) cannot have the same name for the same functionality without being misleading, and that means that the various smart pointers have different names for the same functionality, leading to a steeper learning curve for each one. Consistency in interfaces is important.
    Among my names, Clear is my least favourite. It's taken again from the containers, where it means freeing everything, completely emptying the container. Not the best association, but it still indicates a disassociation of the object from whatever is behind it.
    This disassociation is the primary focus of the name Release. It releases the pointer it has a hold on back its origin. It disassociates from the pointer. There's now one owner less of the memory. The two big disadvantages of the name come from its association. First, the auto_ptr class uses release() for real disassociation, i.e. it passes control over the pointer lifetime back to the programmer. That's not what happens for a reference-counted pointer, because the pointer cannot guarantee that no other pointer exists. Second, Microsoft's COM (and by copying, Mozilla's XPCOM) specifies that every object has a method called Release, but due to the internally reference-counted nature of COM objects, there is no nulling out pointers, and definitely no safety.
    So we're left with Reset. Reset returns to the pointer nature of the smart pointer, simply meaning "return the pointer to its original state", that state being null. That's all that Reset means: set the pointer to null. If the pointer happens to manage the memory of the pointee in such a way that the pointee is deleted as a consequence, that's fine. I like this name most of all.
    In such case, I like Release() best of all. Release is used by COM after all, which also has reference counting.

    No, it doesn't. The purpose of smart pointers is to impose an automated memory handling policy. What exactly this policy does depends on the smart pointer.
    Your reference-counted smart pointer has the policy, "As long as at least one pointer to a memory block exists, the memory block is kept alive."
    If main() calls foo() and passes its sole pointer object by non-const reference, it's saying, "Here's my pointer, along with whatever it points to and the power to modify what it points to." If main() needs a guarantee that the memory is still alive after the call, it shouldn't have passed its only pointer to that memory by non-const reference. It should have created a copy of the pointer first.
    I should probably pass it by const reference, but it seems I can't get the class to work with const since the locking and unlocking fails if the functions are const (go figure?).
    My point is, even if main checks if the memory is intact, if it is not, what should it do? It can't do much more other than fail or terminate. In this case, it's a programming error, so an ASSERT will raise when you try to *ptr on a NULL ptr (the class will ASSERT). Then I know something isn't right and go back and find it.

  9. #69
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,895
    Quote Originally Posted by Elysia View Post
    The only problem (or the biggest anyway) I have with that is that you're leaving the function wide open to free the memory and crashing the class. Unless
    Code:
    delete &var;
    ...doesn't work? I've never tried.
    Murphy vs Machiavelli. delete &var; does work, but what sane programmer would do it? This works, too, and I'm pretty sure it will ruin your day:
    Code:
    void foo(pp<int> ptr)
    {
      std::memset(&ptr, 0, sizeof(pp<int>));
    }
    At the very least, you'll get a memory leak.

    I should probably pass it by const reference, but it seems I can't get the class to work with const since the locking and unlocking fails if the functions are const (go figure?).
    Strange, but a problem in your implementation, not in the principle. (Perhaps there are some members you should make mutable?)

    My point is, even if main checks if the memory is intact, if it is not, what should it do?
    No idea, because that's a domain-specific problem. It could say, "Ah, well, foo() decided that I shouldn't use this pointer anymore, so let's do something else."
    Note that, because main() gave foo() control over the pointer, it probably knows what to do. If it can't do anything if the pointer is erased, then it shouldn't have given foo() control over it in the first place. It should have created its own copy, or it should have called a version of foo() that doesn't modify its argument.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  10. #70
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    Code:
    	CCriticalSection cSync;
    	void Lock() const
    	{
    		if (bThreadSafe)
    			cSync.Lock();
    		else
    			ASSERT(GetCurrentThreadId() == dwThreadId); // If the class is not put into multi-thread safe mode and another thread is trying to use the class, then ASSERT.
    	}
    	void Unlock() const
    	{
    		if (bThreadSafe);
    			cSync.Unlock()
    		else
    			ASSERT(GetCurrentThreadId() == dwThreadId); // If the class is not put into multi-thread safe mode and another thread is trying to use the class, then ASSERT.
    	}
    Compiler error:
    error C2663: 'CCriticalSection::Lock' : 2 overloads have no legal conversion for 'this' pointer

    If I understand that correctly, it's because Lock() isn't declared const. So what's the verdict on that one? A way to get around / use another class/library/function?

  11. #71
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    hmm... but why should Lock() be const in the first place? It changes the observable state of the object. The same goes for Unlock().
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  12. #72
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,895
    The verdict is in abachler's signature: "Friends don't let friends use MFC."

    Seriously, though, the fact that the critical section is affected by the smart pointers constness indicates that you have one per smart pointer. Is that true? If it is, that's completely useless. Allocate a CCriticalSection next to your reference count, because like it, it must be shared among all instances that refer to the same pointer.
    Once you've done that, constness will no longer be a problem.

    Also, the thread affinity test should be done whether the pointer is thread-safe or not, because just because you can have more than one pointer to the same object on different threads, still doesn't mean you may use any single pointer on more than one thread at the same time. That's still dangerous.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  13. #73
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    Quote Originally Posted by CornedBee View Post
    Seriously, though, the fact that the critical section is affected by the smart pointers constness indicates that you have one per smart pointer. Is that true? If it is, that's completely useless. Allocate a CCriticalSection next to your reference count, because like it, it must be shared among all instances that refer to the same pointer.
    Once you've done that, constness will no longer be a problem.
    There are two problems here:
    A critical section is needed to make sure to two or more threads use the same smart pointer class at once (references via threads, anyone?). Unless, of course, I can remove that practice. Two threads should NOT use the same smart class pointer.
    Code:
    // Let's just assume for simplicity's sake that main calls both thread1 and thread2 in different threads.
    void thread1(pp<int>& pTest)
    {
    	int n = *pTest; // BAD!
    }
    void thread2(pp<int> pTest)
    {
    	int n = *pTest; // Good, because it's not the same smart pointer
    }
    int main()
    {
    	ppnew<int> p = 0;
    	thread1(p);
    	thread2(p);
    }
    Also, the thread affinity test should be done whether the pointer is thread-safe or not, because just because you can have more than one pointer to the same object on different threads, still doesn't mean you may use any single pointer on more than one thread at the same time. That's still dangerous.
    I guess that answers my question about the above reference count, huh? I'll make some changes.

  14. #74
    and the hat of sweating
    Join Date
    Aug 2007
    Location
    Toronto, ON
    Posts
    3,545
    I should probably pass it by const reference, but it seems I can't get the class to work with const since the locking and unlocking fails if the functions are const (go figure?).
    Why not define your mutexes to be mutable?

  15. #75
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,895
    Because that would just hide the underlying conceptual mistake.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Pointers v References
    By m37h0d in forum C++ Programming
    Replies: 28
    Last Post: 06-30-2008, 01:29 PM
  2. Replies: 12
    Last Post: 12-31-2007, 06:59 AM
  3. Pointers, Classes, and Errors o my!
    By Scottc1988 in forum C++ Programming
    Replies: 12
    Last Post: 03-13-2003, 10:14 PM
  4. Pointers to derived classes.
    By sean in forum C++ Programming
    Replies: 3
    Last Post: 11-13-2001, 08:19 PM
  5. Pointers to inherited classes
    By sean in forum C++ Programming
    Replies: 1
    Last Post: 11-03-2001, 03:04 PM