Andre Poenitz wrote:
On Tue, Oct 02, 2007 at 08:11:47AM +0200, Abdelrazak Younes wrote:
Andre Poenitz wrote:
The attached patch replaces the signal/slot connections between Buffer
and BufferView to LyXView by ordinary delegates.
There has always been only (at most) a single connection of each type
with known endpoints, so full-blown signal/slot was overkill anyway -
I am sorry but that's not true for Buffer. With multi-workarea and
multi-view there can be more than one connection to the
Buffer::changed() signal.
The signal was only attached to the LyXView, not to the workarea.
Wrong, in WorkArea.h:
/// buffer changed signal connection
boost::signals::connection bufferChangedConnection_;
/// buffer closing signal connection
boost::signals::connection bufferClosingConnection_;
I did not change the structure of the code.
You did a bit I'm afraid.
at least given the price of boost's signal/slot:
As boost/signal pulls in roughly 50 kLOC into both Buffer.h and
BufferView.h which in turn heavily used throughout the code.
So the change reduces "TLOCC" from 24.6M to 22.5M, i.e. by more than 9%!
There might be a few simplifications possible after that, but I wanted
to keep the patch small.
Please wait with that until we understand the full repercussion of that
change. I really sympathize with your compil time reduction goal but I
don't think this is the right solution:
* For one, you are working around a compiler that do not implement pch
efficiently. I am pretty sure MSVC will not be affected much by your patch.
But I am not compiling with MSVC, and if MSVC is "not (adversely)
affected" that's completely sufficient. [Apart from that: also MSVC
will benefit a bit by not having certain stuff to compile into pch,
and finally by not having to compile boost/signal at all]
As I said, I agree with that goal but your solution is rather radical.
* Second, did you tried a more recent version of gcc? Gcc 4.2 seems good
on the paper.
No, I haven't. But I don't think I will. We have been promised "good
gcc" for 15 years or so. I don't believe that anymore.
I can't help you here :-)
* Third, I don't know if you remember but I worked an awful lot to get
rid of the WorkArea and LyXView dependency in the core. You are
basically putting them back :-(
Heck, no. As I said: I did not change the structure of the code. All
dependencies that are in the code now have been there before.
And also note that I did use separate interfaces for the delegates (_even_
if only LyXView implements them) exactly for that reason.
I understand that (except for Buffer::changed() and closing()) and
that's actually what I said just here:
On the third point, I understand this is not exactly the same thing. The
problem with boost signals is that they need full visibility of the
objects so we cannot put them in a private implementation. OK, so let's
focus on the signal solution itself:
- BufferView: I think that most if not all the signal could go with some
more cleanup. This is the right way forward IMO.
- Buffer: only the changed() signal is important. One solution would be
to have a centralized Buffer manager that will track the WorkAreas
connected to it and update them if need be. This will avoid the need for
this signal in particular. All other signals could and should be removed.
I hope you are not too upset with my reaction.
I am not. I read that that I am 80% going in the right direction, and
the remaining bit (multiple buffers) would need a new implementation
anyway as we are currently connecting via (a single!) LyXView to the
current WorkArea only.
Wrong, see above. You can have two WorkAreas sharing the same Buffer in
the same LyXView. I'd really like this to stay.
The idea of having a BufferManager (which I agree
with) is not affected with that patch.
OK.
Abdel.
