1

I have classes Graph and Algorithm:

class Graph
{
...
};

class Algorithm
{
...
private:
Graph * mGraph;
...
};

I want my algorithm to be able to do everything with mGraph except deleting it. I.e. I want to detect (at compile time) if somewhere in algorithm I'm deleting the graph. Is there a good (elegant) way to do such a thing? (The only way I realized is making Graphs destructor private so only friend classes have permission to delete it)

Marc Mutz - mmutz
  • 24,485
  • 12
  • 80
  • 90
Mihran Hovsepyan
  • 10,810
  • 14
  • 61
  • 111
  • Can a graph be initialised by an external entity a reference passed to Algorithm? We need to know more about the specifics of Graphs use. – cmannett85 Apr 30 '11 at 07:58
  • 2
    How can you delete something at compile-time? `delete` is a runtime construct. – Nawaz Apr 30 '11 at 07:58
  • 1
    @Nawaz I mean solving this problem via access level specificators. – Mihran Hovsepyan Apr 30 '11 at 08:04
  • 4
    Concentrate on preventing Murphy from wreaking havoc. You can never truly protect against Machiavelli. That's a private data member. If you can't trust the members of `Algorithm` to not to abuse it, you need to split `Algorithm` into several classes. – sbi Apr 30 '11 at 09:25

4 Answers4

6

As others have pointed out, you cannot guard against malicious users of Graph. I'll assume that you don't want to, either, and that you're in the position of the Graph author trying to prevent users of the class from misunderstanding the ownership semantics of Graph while implementing Algorithm(s).

You could hand out only shared_ptr<Guard> by using the Named Constructor Idiom, which should prevent all but the most clueless of coders from attempting to delete Graph, however, Algorithm writers could still use Graph * mGraph as a member variable and pass shared_ptr<>::get() when constructing it...

To make this watertight for all but the most determined of malicious coders, you need to use the Counted Body Idiom. In short: wrap Graph*s with a GraphHandle class that is passed by-value and proxies Graph's API. It's a bit like only ever creating shared_ptr<Graph>s, but it prevents access to the raw pointer:

class Graph {
public:
    // ...
    void doSomething();
};

class GraphHandle {
    shared_ptr<Graph> graph;
public:
    explicit GraphHandle( const shared_ptr<Graph> & graph )
        : graph( graph )
    {
        assert( graph );
    }
    // do NOT provide an accessor for this->graph!
    // proxied API:
    void doSomething() {
        graph->doSomething();
    }
    // ...
};

class Algorithm {
    // ...
    GraphHandle graph;
};

This way, Algorithm can no longer delete the Graph.

Algorithm authors can of course still use Graph directly. To prevent this, make Graph private API and hide it completely behind GraphHandle. You can see this in production in the DOM implementation of Qt.

As an aside:

Using shared_ptr in the implementation of GraphHandle doesn't mean GraphHandle necessarily owns the Graph. You can hold a shared_ptr<Graph> outside of the GraphHandle, or, if the rest of the code uses naked pointers, you can just pass nodelete as the shared_ptrs deleter:

struct nodelete {
    template <typename T>
    void operator()( T* ) {}
};

// ...
Graph * nakedGraph = ...;
const shared_ptr<Graph> sharedGraph( nakedGraph, nodelete() );
GraphHandle handledGraph( sharedGraph );
Marc Mutz - mmutz
  • 24,485
  • 12
  • 80
  • 90
  • I like this solution! But here is some problem, I don't wan't the graph to be deleted after algorithm is deleted. So maybe it's good to not have smart pointer in `GraphHandle` but just pointer. – Mihran Hovsepyan Apr 30 '11 at 08:10
  • Thanks! But this is becoming very unreadable, so for me just pointer is ok :) – Mihran Hovsepyan Apr 30 '11 at 08:18
  • If we assume, to the point we must actively protect against it, a misguided coder will delete mGraph in Algorithm, why don't we assume someone will incorrectly modify GraphHandle to similarly do the wrong thing? – Thomas Edleson Apr 30 '11 at 09:02
  • 1
    `delete &*graph` will pull out the object from under the smart pointer without the latter noticing. This is futile. Seen my comment to the question. – sbi Apr 30 '11 at 09:27
  • @Thomas: I guess OP's concern is that while `Graph` (and therefore, potentially, `GraphHandle`) is developed by someone how knows his way around C++, `Algorithm`s may be coded by not-so-proficient coders. If you assume that the `Graph` definition is up for grabs, there's indeed nothing that we can do. `Graph` might not be public API, in case you worry that the `Algorithm` coder will just continue to use `Graph*` as a member. "Good library design can deal with this" is the gist of it, I guess. – Marc Mutz - mmutz Apr 30 '11 at 09:39
  • 1
    @sbi: You are right. Note, however, that the `shared_ptr` is _private_ in `GraphHandle`, and that I specifically mentioned `// do NOT provide an accessor to this->graph`. – Marc Mutz - mmutz Apr 30 '11 at 09:42
  • 2
    @mmutz: I certainly agree that keeping the object in a smart pointer is better than in a naked pointer _if_ it's been owned by that pointer (which we don't know), but 1) the thing has been `private` before and 2) that comment could have been applied also, so both are not an argument in favor of your solution. – sbi Apr 30 '11 at 09:57
  • @sbi: for both your comments, I think you simply misread my post. Sorry if I was unclear. I'll try editing to clean it up. – Marc Mutz - mmutz Apr 30 '11 at 10:16
  • @mmutz: I see now that your idea is a bit different than I thought, but it's still nonsense, because your solution requires all algorithm in `Algorithm` to be doubled in `GraphHandle`. Not only is that redundant, it also requires all algorithms to be implemented in `GraphHandle` - _where they again have access to the underlying pointer_! And that's not a weakness of your implementation either, since, ultimately, all algorithms need to work with the underlying graph, and thus need access to the actual `Graph` object. Really, just face it: this is futile, Machiavelli will always find a way. – sbi May 01 '11 at 07:41
  • @sbi: I kindly request complaints about the usefulness of the [Proxy Pattern](http://en.wikipedia.org/wiki/Proxy_pattern) to be directed at the [GoF](http://en.wikipedia.org/wiki/Design_Patterns_(book)) directly, thank you :) – Marc Mutz - mmutz May 01 '11 at 07:47
  • @mmutz: Please read my comment again. I know the Proxy Pattern, I know what it is for, and I know that it involves duplicating an interface. However, in your case you're doing it in order to restrict access to a private data member, but don't achieve it, because the algorithms still need to access it. – sbi May 01 '11 at 18:34
  • @sbi: What I don't get: if we agree that `GraphHandle`'s API is essentially the same as the original `Graph`s (as per Proxy Pattern), then what makes you think that `Algorithm`s still need `Graph` directly, or that they need to be implemented "in `GraphHandle`"? In `Algorithm`s, you just `Graph*`->`GraphHandle`... and `mGraph->doSomething()`->`mGraph.doSomething()`. Where's the "need [for] access to the actual `Graph` object"?? – Marc Mutz - mmutz May 01 '11 at 19:05
  • @mmutz: You need to implement the algorithms in `GraphHandle`, and have `Algorithm` call those. `Algorithm::doSomething()` will call `GraphHandle::doSomething()`, which will have to do the actual work, because in order to do something with a graph, you will need the graph. And that brings you back to square one, only with another layer of Patterns wrapped around the thing. – sbi May 01 '11 at 20:37
  • @sbi: I'm getting tired... _again_: where's the need to access the actual `Graph` object when all public `Graph` API is provided by `GraphHandle`, too (as per _definition of Proxy_)? E.g.: if you want to call `Graph::blah()` _there will be a_ `GraphHandle::blah()` _that calls_ `graph->blah()`. There is _no_ need for `Algorithm` to access `Graph` - all it could _ever_ want to do with (the public API of) `Graph`, it can do with `GraphHandle` — except delete the underlying `Graph`. – Marc Mutz - mmutz May 01 '11 at 20:48
2

You can't stop a malicious user from wreaking havoc, and your energies would be better spent documenting and educating users of the Graph class on how it should be used.

Raw pointers are used in many different ways, so not using a raw pointer would help self-document the expected lifetime. A reference may be appropriate in this case, since it sounds like 1) the lifetime is completely managed outside of the Algorithm class, and 2) Algorithm doesn't need to change which Graph it has. Otherwise a smart pointer may be appropriate, depending on what the actual lifetime is expected to be.

However, since mGraph is private, its presence can be completely encapsulated within Algorithm. Then all you need to do is document Algorithm's class invariant (which you should be doing anyway) and clearly specify that mGraph should not be deleted. Relatively few people should be touching Algorithm's internals (usually fewer than use the public interface), and they are expected to know the details of how Algorithm is implemented.

Docs and comments are the often overlooked, yet often most powerful tool we have against code misuse.

Thomas Edleson
  • 2,175
  • 1
  • 16
  • 19
1

You could give Graph a private operator delete and friend the classes that are allowed to delete it. See here for information on making it private: Which operator delete?

The friended class could be a custom deleter you could give to a smart pointer (like boost::shared_ptr).

Community
  • 1
  • 1
Pablo
  • 8,644
  • 2
  • 39
  • 29
  • as you can see in my questio this idea I already have `(The only way I realized is making Graphs destructor private so only friend classes have permission to delete it)` – Mihran Hovsepyan Apr 30 '11 at 08:06
  • I didn't mean the destructor, I meant `operator delete`. If the destructor is still public, you can still create instances of `Graph` on the stack without a problem. It's only when trying to `delete` an instance that the access restriction applies. – Pablo Apr 30 '11 at 08:08
  • +1 Good point! But think for me this is the same, because all my graphs in heap. – Mihran Hovsepyan Apr 30 '11 at 08:12
  • `::delete mGraph` You can't stop braindead code(rs) from wreaking havoc. – Thomas Edleson Apr 30 '11 at 08:52
0

Making Graph's destructor private would indeed stop it from being deleted by Algorithm.

You can't make Graph's pointer const, as you can delete through const pointers.

If you held a reference to Graph, you could only call its destructor by jumping through hoops:

Graph* graphPtr = &mGraph;
delete graphPtr;

You could have the same effect with some kind of reference-counting smart pointer.

tenpn
  • 4,556
  • 5
  • 43
  • 63