Re: [Libreoffice] [PATCH] Easyhack fdo#38831 - Convert some SvStrings to std::vector

Hi,

> > > Attached are 2 patches to covert come SvStrings to std::vector.
> >
> > I realised I missed some size_t conversions and attached is an updated
> > version of the second patch (the first patch is fine).
>
> Great stuff, pushed both, thank you! :-)
>
> http://cgit.freedesktop.org/libreoffice/core/commit/?id=d8f2a82f6905178f1f594b22a0d5427b29c8eb33
> http://cgit.freedesktop.org/libreoffice/core/commit/?id=a9b3b64a5a94a4c27ac524ac6997ef2e2467267c
>
> I've only read the code, because I was unable to locate the affected
> functionality - can you please point me to where exactly should I click
> to execute this functionality? ;-)

Good question :-) I traced it back to the AutoText feature in Writer 
i.e. Edit -> AutoText. The "Categories" button lets you 
create/rename/delete groups and if you highlight some text in the 
document before selecting Edit -> AutoText you can create a new entry.

I stumbled on at least one bug that is also present in my distro 3.4.4 
build.

* Highlight some text in the main document.
* Edit -> AutoText
* Click "Categories" and create a new group ("TestGroup").
* Under TestGroup create a new entry called "TestEntry" via the
  AutoText -> New button.
* Click on the newly created "TestEntry" and then click back on
  "TestGroup".
* I believe the AutoText button now incorrectly displays rename/delete
  etc.
* Click rename and and a dialog appears for TestEntry and not
  TestGroup. Rename TestEntry to TestEntry2 and click Ok.
* The group list is now messed up with the entry name where the group
  name should be.
* Keep clicking the various group names and within 10-20s writer will
  crash (reproduced numerous times).

I'll open a bug report.

> Other than that, aren't we leaking strings in pInsertedArr->erase(it) in
> the first patch?  But it seems to me that it we were leaking even with
> the old code, so I applied that anyway :-) - but would be great if you
> can double-check / fix.

Yeah, it looks like all 3 original Remove() calls resulted in leaks. The 
attached patch should fix this.

> > Any comments on the size_t changes/best practice welcomed.
>
> I am not a particular friend of the sal_uInt16 kind of types, but
> changing all that to size_t where the size() of the vector might be
> returned is counter-productive too, so I think you did the right thing -
> casted where necessary.

I realised afterwards that I added some unnecessary casts as comparing 
sal_uInt16 and size_t should be fine as they are both unsigned ints e.g.

+    if( static_cast<size_t>(nNewPath) >= m_vPathArr.size() )

I will omit these unnecessary casts in the future to avoid uglifying the 
code.

Regards,
Brad

> Thank you,
> Kendy

From c1f6ed366747d9d8db12785978db3a8155478afd Mon Sep 17 00:00:00 2001
From: Brad Sowden <code@sowden.org>
Date: Sat, 31 Dec 2011 11:29:34 +1300
Subject: [PATCH] Fix memory leaks

---
 sw/source/ui/misc/glosbib.cxx |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/sw/source/ui/misc/glosbib.cxx b/sw/source/ui/misc/glosbib.cxx
index fff2a2c..3df5186 100644
--- a/sw/source/ui/misc/glosbib.cxx
+++ b/sw/source/ui/misc/glosbib.cxx
@@ -294,6 +294,7 @@ IMPL_LINK( SwGlossaryGroupDlg, DeleteHdl, Button*, pButton  )
         {
             if( **it == sEntry )
             {
+                delete *it;
                 pInsertedArr->erase(it);
                 bDelete = sal_False;
                 break;
@@ -310,6 +311,7 @@ IMPL_LINK( SwGlossaryGroupDlg, DeleteHdl, Button*, pButton  )
             {
                 if( (*it)->GetToken(0, RENAME_TOKEN_DELIM) == sEntry )
                 {
+                    delete *it;
                     pRenamedArr->erase(it);
                     bDelete = sal_False;
                     break;
@@ -357,6 +359,7 @@ IMPL_LINK( SwGlossaryGroupDlg, RenameHdl, Button *, EMPTYARG )
         {
             if( **it == sEntry )
             {
+                delete *it;
                 pInsertedArr->erase(it);
                 pInsertedArr->push_back(new String(sNewName));
                 bDone = sal_True;
-- 
1.7.7.4