Abdelrazak Younes wrote:
rgheck wrote:
Abdelrazak Younes wrote:
rgheck wrote:
Abdelrazak Younes wrote:
rgheck wrote:
Andre Poenitz wrote:
But if the idea is to store the params _because they change_,
that should be a copy, i.e.
otherstack.top().bparams = new BufferParams(buffer_.params())
with a corresponing delete somewhere, shouldn't it?
Well, here's a patch I suggested previously. JMarc thought it
might be better to use a shared_ptr here. I have no strong views.
You?
Isn't the attached patch simpler?
This doesn't make a copy of the BufferParams but just stores a
pointer to the old one. It has to make a copy.
Isn't that what I do here?
+ otherstack.top().bparams = new BufferParams(buffer_.params());
It has to make a copy in the constructor, whereas:
MathData * ar, BufferParams const & bp,
bool ifb) :
kind(kin), cursor(cur), cell(cel), from(fro), end(en),
- pars(pl), array(ar), bparams(bp), isFullBuffer(ifb)
+ pars(pl), array(ar), bparams(&bp), isFullBuffer(ifb)
{}
/// Which kind of operation are we recording for?
UndoKind kind;
@@ -90,7 +90,7 @@
/// the contents of the saved MathData (for mathed)
MathData * array;
/// Only used in case of full backups
- BufferParams bparams;
+ BufferParams const * bparams;
/// Only used in case of full backups
bool isFullBuffer;
you only store a pointer to the original BufferParams.
I still don't understand (or you don't ;-)). The purpose of my patch
was to avoid making a copy of the BufferParams in the case of a
non-full undo or redo. In that case storing a pointer to the original
BufferParams is fine. In the cas of a full buffer undo/redo operation
on the contrary we want to make a full copy. So making a copy in the
constuctor is exactly what I don't want to do because this will then
no different than storing a BufferParams instance as we do now.
OK, I see. Well, I think it works that way, but only because
doRecordUndo() only gets called with isFullBuffer being true at this one
point in the code. If it ever got called that way from elsewhere, you'd
have a problem. So it seems to me to be clearer to do this in the
constructor.
And while we're on micro-optimization ;~/, how about the attached? Saves
a copy.
rh
Index: Undo.cpp
===================================================================
--- Undo.cpp (revision 25540)
+++ Undo.cpp (working copy)
@@ -304,8 +304,8 @@
UndoElementStack & otherstack = isUndoOperation ? redostack_ :
undostack_;
// Adjust undo stack and get hold of current undo data.
- UndoElement undo = stack.top();
- stack.pop();
+ UndoElement & undo = stack.top();
+ // We'll pop it only when we're done with it.
// We will store in otherstack the part of the document under 'undo'
DocIterator cell_dit = undo.cell.asDocIterator(&buffer_.inset());
@@ -368,6 +368,8 @@
LASSERT(undo.array == 0, /**/);
cur = undo.cursor.asDocIterator(&buffer_.inset());
+ // Now that we're done with undo, we can pop it off the stack
+ stack.pop();
if (labelsUpdateNeeded)
updateLabels(buffer_);
