[fpc-devel] Questions on TStringList.Find change (Mantis 28744)

[David Jenkins]

On 3/22/16 2:01 AM, Michael Van Canneyt wrote:
> But you agree that if Find is called on an unsorted list, bogus data will
> be returned ? Or, worse, an never-ending loop may occur ?
> In short, as you say, an "error condition" can ensue without you 
> knowing it.
>
> That is a bug. So, in my opinion (and of some others), Find is 
> implemented buggy in Delphi.
>
> Consider the buggyness a given in what follows.
>
> If we consider something a bug in Delphi, we do not copy the bug.
>
> You happen to rely on this buggy behaviour. This is of course 
> unfortunate.
>
> I attempted to fix this bug using a not too invasive method.
> Originally I simply wanted to raise an exception, but that was deemed too
> invasive.
>
> There are now several options:
> 1. I change the code to raise an exception instead (as was my original 
> plan)
> 2. I change the code to resort to a linear search if sorted is false.
> 3. I introduce a boolean property that determines whether Find does 
> any check.
>    By default the check will be done, to warn people they're doing
>    something stupid. If they override the check, we can assume they 
> know what
>    they are doing.
> 4. Combine 1 and 3.
>
> Or,
>
> 5. You override sort so it does nothing. It is virtual.
>    You can then set sorted to True, no sort will occur, and find will 
> do it's job.
>
> 6. You use another TStringList where you change find to suit your needs.
>    All it takes for you is to copy the implementation, you can keep 
> the same
>    name, and insert the unit after the classes unit in places where 
> you need it.
>
> It's your call which of the 5 methods is used.
>
> But one thing is sure: we're not reverting the behaviour back to the 
> buggy behaviour.
>
> Michael.
> _______________________________________________
> fpc-devel maillist  -  fpc-devel at lists.freepascal.org
> http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel
#1 is what Zoe Peterson (my coworker) has suggested as well. Without 
that exception (as the code now is) there is no warning issued just an 
ambiguous return of False.  Which means that the code is still buggy as 
the user could assume that the Index value points to next insert spot 
and instead it is out of range and causes a crash.   Setting Index value 
to -1 would be a better indicator of bad usage than just returning False 
(with raising an exception still being the best warning).

Keep in mind though that being able to call find on a manually sorted 
list (Sorted <> true) is not buggy behavior (this is the Delphi 
compatibility that we are interested in maintaining).  In which case 
adding in #3 would be necessary.  So I guess I am for #4.

I would suggest that the added boolean flag be named to indicate that 
the list is manually sorted (ManualSorted ?) rather than the flag being 
just a 'bypass check' indicator.  The check in Find would be "if not 
Sorted or not ManualSorted then raise exception'.  But that's just a 
personal preference and not something that I feel extra committed to.

My second choice would be #1 by itself.

David