Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicated checksum code #13

Closed
delphidabbler opened this issue Nov 2, 2022 · 3 comments
Closed

Duplicated checksum code #13

delphidabbler opened this issue Nov 2, 2022 · 3 comments
Assignees
Labels
completed Work completed and merged into develop on remote. Will close when merged to master refactoring

Comments

@delphidabbler
Copy link
Owner

function TBinaryPatchWriter.CheckSum(Data: PCChar; Length: Cardinal): Longint;

and

class function CheckSum(Data: PAnsiChar; DataSize: Cardinal;

have similar, but not identical code:

class function TPatcher.CheckSum(Data: PAnsiChar; DataSize: Cardinal;
  const BFCheckSum: Integer): Longint;
begin
  Result := BFCheckSum;
  while DataSize <> 0 do
  begin
    Dec(DataSize);
    Result := ((Result shr 30) and 3) or (Result shl 2);
    Result := Result xor PShortInt(Data)^;
    Inc(Data);
  end;
end;

function TBinaryPatchWriter.CheckSum(Data: PCChar; Length: Cardinal): Longint;
begin
  Result := 0;
  while Length <> 0 do
  begin
    Dec(Length);
    Result := ((Result shr 30) and 3) or (Result shl 2);
    Result := Result xor Ord(Data^);
    Inc(Data);
  end;
end;

For the avoidance of error could the calculation part of the loops be extracted into a utility function? Such a function would need to be passed a brought forward checksum and the two types of Data parameters would need to be cast to ShortInt (= Int8).

Could be something like:

function UpdateChecksum(BF: Longint; Value: Int8): Longint;
begin
  Result := ((Result shr 30) and 3) or (Result shl 2);
  Result := Result xor Value;
end;

or

procedure UpdateChecksum(var CheckSum: Longint; Value: Int8);
begin
  CheckSum := (CheckSum shr 30) and 3) or (CheckSum shl 2);
  CheckSum := CheckSum xor Value;
end;
@delphidabbler delphidabbler self-assigned this Nov 2, 2022
@delphidabbler delphidabbler added the considering Under consideration label Nov 2, 2022
@delphidabbler delphidabbler changed the title Duplicate checksum code Duplicated checksum code Nov 5, 2022
@delphidabbler
Copy link
Owner Author

How about a class, TCheckSum, that calculates a checksum for all the bytes that are added to it. The class could be passed a starting checksum in its constructor.

Something like:

type
  TCheckSum = class
  private
    fCheckSum: Longint;
  public
    constructor Create(Seed: Longint);
    procedure Add(Value: Int8);
    property CheckSum: Longint read fCheckSum;
  end;

Implemented as:

procedure TCheckSum.Add(Value: Int8);
begin
  fCheckSum := (fCheckSum shr 30) and 3) or (fCheckSum shl 2);
  fCheckSum := fCheckSum xor Value;
end;

constructor TCheckSum.Create(Seed: Longint);
begin
  inherited Create;
  fCheckSum := Seed;
end;

Our two pieces of code that calculate checksums then become:

class function TPatcher.CheckSum(Data: PAnsiChar; DataSize: Cardinal;
  const BFCheckSum: Integer): Longint;
begin
  var CS := TCheckSum.Create(BFCheckSum);
  try
    while DataSize <> 0 do
    begin
      Dec(DataSize);
      CS.Add(PShortInt(Data)^);
      Inc(Data);
    end;
    Result := CS.CheckSum;
  finally
    CS.Free;
  end;
end;

function TBinaryPatchWriter.CheckSum(Data: PCChar; Length: Cardinal): Longint;
begin
  var CS := TCheckSum.Create(0);
  try
    while Length <> 0 do
    begin
      Dec(Length);
      CS.Add(Ord(Data^));
      Inc(Data);
    end;
    Result := CS.CheckSum;
  finally
    CS.Free;
  end;
end;

It may be possible to run the loop from within TCheckSum if either the calling methods create an array of Int8 (or a buffer) to pass into TCheckSum, or the length of data is passed to TCheckSum and there's a callback function for each data item.

@delphidabbler
Copy link
Owner Author

It may be possible to run the loop from within TCheckSum if either the calling methods create an array of Int8 (or a buffer) to pass into TCheckSum, or the length of data is passed to TCheckSum and there's a callback function for each data item.

Extending TCheckSum from #13 (comment), we could have:

type
  TCheckSum = class
  // ...
  public
    // ...
    procedure AddBuffer(Data: PInt8; Length: Cardinal);
    // ...
  end;

// ...

procedure TCheckSum.AddBuffer(Data: PInt8; Length: Cardinal);
begin
  while Length > 0 do
  begin
    Dec(Length);
    Add(Data^);
    Inc(Data);
  end;
end;

Now the calling code reduces to:

class function TPatcher.CheckSum(Data: PAnsiChar; DataSize: Cardinal;
  const BFCheckSum: Integer): Longint;
begin
  var CS := TCheckSum.Create(BFCheckSum);
  try
    CS.AddBuffer(PInt8(Data), DataSize);
    Result := CS.CheckSum;
  finally
    CS.Free;
  end;
end;

function TBinaryPatchWriter.CheckSum(Data: PCChar; Length: Cardinal): Longint;
begin
  var CS := TCheckSum.Create(0);
  try
    CS.AddBuffer(PInt8(Data), Length);
    Result := CS.CheckSum;
  finally
    CS.Free;
  end;
end;

@delphidabbler delphidabbler added accepted and removed considering Under consideration labels Nov 7, 2022
@delphidabbler
Copy link
Owner Author

var CS := TCheckSum.Create(BFCheckSum);

If this code is implemented before conversion to Delphi 11 (issue #12), then occurrences of inline variable declarations will need to be changed to use to separate the var declaration & initialisation.

@delphidabbler delphidabbler added this to the Next Release milestone Nov 8, 2022
@delphidabbler delphidabbler added completed Work completed and merged into develop on remote. Will close when merged to master and removed accepted labels Mar 28, 2023
@delphidabbler delphidabbler removed this from the Next Release milestone May 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completed Work completed and merged into develop on remote. Will close when merged to master refactoring
Projects
None yet
Development

No branches or pull requests

1 participant