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

[Brad Sowden]

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
>