Coding practice: OK to leave dangling pointer if not currently used?

[Scott Kostyshak]

A commit that I introduced earlier caused crashes and I had to revert.
The reason was because I used a dangling pointer. It did not occur to
me that the pointer could be dangling. I just assumed it would not be
(I've learned my lesson). The reason why it becomes dangling is
because I add code after a dispatch and thus the buffer view might
have been destroyed (e.g. from closing a buffer or closing a view).

My solution is to simply refresh the pointer (I can get it from
currentBufferView()). However, this got me thinking: shouldn't we set
the pointer to null every time we know that it was destroyed? What if
we don't use that pointer again (as was the case before my commit)?
Looking back, it is pretty obvious that the pointer could have become
dangling and I should have realized this potential problem.

I guess I have two questions:
(1) Are there any comments on my patch?

(2) Even if I did not want to add code to GuiView::dispatch, would it
have been a reasonable patch to _only_ refresh the pointer after the
dispatch, just in case someone wanted to add code to it eventually?

Scott

[Richard Heck]

Post by Scott Kostyshak
(1) Are there any comments on my patch?
diff --git a/src/frontends/qt4/GuiView.cpp b/src/frontends/qt4/GuiView.cpp
index b690ac1..ad00659 100644
--- a/src/frontends/qt4/GuiView.cpp
+++ b/src/frontends/qt4/GuiView.cpp
@@ -3881,6 +3881,18 @@ void GuiView::dispatch(FuncRequest const & cmd, DispatchResult & dr)
if (menuBar()->isVisible() && lyxrc.full_screen_menubar)
menuBar()->hide();
}
+
+ // Need to update bv because many LFUNs here might have destroyed it
+ bv = currentBufferView();
+
+ // Clear non-empty selections
+ // (e.g. from a "char-forward-select" followed by "char-backward-select")
+ if (bv) {
+ Cursor & cur = bv->cursor();
+ if ((cur.selection() && cur.selBegin() == cur.selEnd())) {
+ cur.clearSelection();
+ }
+ }
}

What about checking if bv has changed? If it has, then probably we do
not want to clear the selection.

I.e., something like:

BufferView * newbv = currentBufferView();
if (newbv == bv) {
...
}

We know that bv was previously non-zero, right?

It seems reasonable to set a pointer to null when you destroy it. But of
course, there may still be other pointers around to the same memory.
That may be true in this case.

Richard

[Jean-Marc Lasgouttes]

Post by Richard Heck
What about checking if bv has changed? If it has, then probably we do
not want to clear the selection.

We do not really care about that. We only want to have a correct
selection (even if the bv changed).


JMarc