Do code review before v1.0.0 release

[delphidabbler]

Code would benefit from a review before committing to v1 release.

[delphidabbler]

As a first example, in BDiff.BlockSort.pas the following code could potentially be buggy if Len is negative before the loop:

class function TBlockSort.Compare(A, B: Int32; Data: PCCharArray;
  DataSize: Int32): Int32;
begin
  var PA: PCChar := @Data[A];
  var PB: PCChar := @Data[B];
  var Len: Int32 := DataSize - A;
  if DataSize - B < Len then
    Len := DataSize - B;
  while (Len <> 0) and (PA^ = PB^) do
    ...

I think a better test on the while loop would be:

  while (Len > 0) and (PA^ = PB^) do
    ...

And, while we're at it, would setting Len be clearer if

  var Len: Int32 := DataSize - A;
  if DataSize - B < Len then
    Len := DataSize - B;

was replaced by

  var Len: Int32 := DataSize - Max(A, B);

Finally, we should assert that Data <> nil A >= 0, B >= 0 and DataSize > 0, and that A <= DataSize and B <= DataSize. The assertions on the integers will be sufficient to be certain that Len >= 0.

[delphidabbler]

class function TBlockSort.Execute(Data: PCCharArray; DataSize: Int32): PBlock;
begin
  if DataSize = 0 then
    Exit(nil);
  GetMem(Result, SizeOf(Int32) * DataSize);
  ...

In the above, DataSize could, in theory, be negative resulting in an invalid memory allocation. This shouldn't happen, but a simple assertion would guard against it.

We should also assert that Data <> nil.

Given that this method is public I propose that the assertions remain in the release code, while assertions in the private methods could be debug only.

Method comments need to be revised to note that the method will return nil if DataSize = 0.

[delphidabbler]

In

 procedure TBlockSort.Sink(Left, Right: Int32; Block: PBlock;
  Data: PCCharArray; DataSize: Int32);

we should probably place assertions that Left >= 0, Right >= 0, DataSize > 0, Block <> nil and Data <> nil. Additionally, we should have Left < DataSize and Right <= DataSize.

[delphidabbler]

TBlockSort could be refactored into a non-static class, with the parameters currently passed to Execute being passed instead to the constructor and stored in fields. To do so would reduce the number of method parameters and reduce the number of checks on the values that are required.