2011/11/28 Michael Stahl <mst...@redhat.com>:
> On 25/11/11 19:05, Ivan Timofeev wrote:
>> Well, another possible memory optimization is to merge
>> SwSortedObjsImpl into SwSortedObjs. What is the reason to use the
>> "pImpl" pattern here?
>
> usually pImpl is a good idea because it is the only way to get actual
> encapsulation in C++; but in this case it definitely looks overdone to me...
>
> it really does not make any sense to have a separate header and cxx file
> for the impl class, that should all be moved into swsortedobjs.cxx.
>
> hmmm... am not sure about the question "should it have a pImpl at all",
> currently the impl only contains a vector, but on the other hand,
> perhaps somebody will add other members later... i don't have a strong
> opinion in this case :)
impl here is just wrapper for the vector that only inserts objects in
a right place... So, merged after all :-)
>> Ok, there are many SwFrms in a big doc... We don't want to throw away
>> memory. But we would use a shared between the SwFrm instances static
>> variable for an empty list ("null object"), and return it in
>> GetDrawObjects when our SwSortedObjs is dead. What do you think about
>> it?
>
> for this case i don't like it, because this null object would be
> mutable: it would be possible that somebody mis-uses this to get the
> pDrawObjs, receiving the static instance, and then adding an element,
> which is obviously bogus.
>
> so the explicit null checks look like the lesser evil :)
Ah yes, you are right! ;) I foolishly didn't consider that case.
Leaving it as is.
> what perhaps could be investigated is whether the pDrawObjs really has
> to be a member of SwFrm, perhaps it could be moved to some sub-class
> that is less often used... but note i am not familiar with the layout
> code so that is just a guess.
Heh, I'm afraid that I haven't got the guts to do such analysis. :-)
Thank you for your attention and tips!
All the best,
Ivan
_______________________________________________
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
