[PUSHED] Re: [PATCH 1/2] Replace SvStringsISortDtor in edtwin.cxx and gloslst.[ch]xx

On 04/06/12 14:01, Brad Sowden wrote:
> Hi,
>
> This patch converts a SvStringsISortDtor to a vector and maintains 
> equivalent functionality.

hi Brad,

thanks for the patch, i've pushed it to master.

> Instead of sorting strings upon each insertion all strings are now added 
> and then a single sort and "unique" are applied (SvStringsISortDtor 
> behaves like a SET with regards to insertion, which is why "unique" is 
> applied). SwEditWin::ShowAutoTextCorrectQuickHelp() is the only place 
> where strings are added to the vector.
>
> The change in behaviour from keeping the first inserted version of a 
> string to keeping the first version post-sort (case-insensitive ASCII 
> comparison) should have no material impact as the strings retrieved from 
> SwAutoCompleteWord are already unique (case-insensitive ASCII 
> comparison) and the capitalization of the string is generally changed 
> anyway to match the capitalization of the word to be auto-completed. 
> Also, there appears to be no logical reason to store the first inserted 
> version of a string over the first version post-sort.

hmmm... makes sense.

> * In previous patches that I've seen on the list it appears ok to 
> convert a vector of String pointers to simply a vector of String values 
> so I've done the same.

yes that's a nice improvement.

> * I used a pointer to the vector as the move() function previously 
> copied and cleared all elements whereas a pointer swap would suffice.

ah that's why, i had wondered....
would it be possible to use std::swap on the vector itself rather than
the pointer?   i could imagine that being implemented efficiently, and
it would allow the vector to not be a pointer, which is simpler.

also (but that was a pre-existing condition, not really your fault)
member names should start with "m_" so it's easy to grep for member access.

> * I've put a TODO in the code to replace the ASCII only case-insensitive 
> sort (existing limitation).
>
> - make + make dev-install successful

that's a good start, please try to use "make check", which is a one-stop
command that does everything.  it probably wouldn't have found a problem
in this case, as i doubt there's a good test for autotext, but in
general it's a good idea.

> - functionality tested ok
> - licence statement on file

regards,
 michael