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

Replace use of Cardinal with other types where necessary #10

Closed
delphidabbler opened this issue Jun 18, 2022 · 17 comments
Closed

Replace use of Cardinal with other types where necessary #10

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

Comments

@delphidabbler
Copy link
Owner

BDiff uses Cardinal wherever the original C code used size_t.

This is fine for 32 bit code, where both types are 32 bit, but size_t is 64 bit on 64 bit compilers whereas Cardinal remains 32 bit.

So it could be a good idea to replace Cardinal with NativeUInt, which changes size like size_t depending on the native word size.

This isn't necessary if BDiff is to remain 32 bit, and is not sufficient if BDiff moves to 64 bit, but it will help to smooth the transition if it is made.

@delphidabbler
Copy link
Owner Author

I'm not sure if NativeUInt is defined on Delphi XE, which BDiff is currently developed with.

So some code like the following in UBDiffTypes.pas may be needed:

{$IF not Declared(NativeUInt) }
type
  NativeUInt = Cardinal;
{$IFEND}

Will need to check the above syntax because I'm rusty on it and away don't have Delphi available to check.

@delphidabbler delphidabbler self-assigned this Jun 18, 2022
@delphidabbler delphidabbler added the considering Under consideration label Jun 18, 2022
@delphidabbler
Copy link
Owner Author

Check original C code to check where Cardinal has been used to stand in for size_t. Maybe some uses of Cardinal are still required to be 32 bit.

C code download

@delphidabbler
Copy link
Owner Author

This could get very buggy very quickly. Maybe leave the programs as 32 bit for now?

@delphidabbler
Copy link
Owner Author

Examination of the original C source code suggests that all Cardinal types in

TBlockSort = class(TObject)

were substituted for size_t, and therefore should be changed to NativeUInt.

@delphidabbler
Copy link
Owner Author

All Cardinal types used in methods of

TDiffer = class(TObject)

would seem to be size_t replacements and so need to be NativeUInt.

But the type of the MinMatchLength property can remain Cardinal. In fact UInt16 might suffice. See the maximum size of a numeric parameter before deciding.

@delphidabbler
Copy link
Owner Author

The Size property of

TFileData = class(TObject)

needs to be NativeUInt because the original C code always stored file sizes as size_t.

@delphidabbler
Copy link
Owner Author

All Cardinal types in

unit UPatchWriters;

replaced size_t types and so need to be NativeUInt. This includes the, perhaps less obvious,

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

@delphidabbler
Copy link
Owner Author

Should

class function Seek(Handle: Integer; Offset: Longint; Origin: Integer):

have its Offset &/or Origin parameter data types changed? Need to check if the SysUtils.FileSeek function that the method calls supports 64 bit offsets.

@delphidabbler
Copy link
Owner Author

The Size property of

TFileData = class(TObject)

needs to be NativeUInt because the original C code always stored file sizes as size_t.

Maybe UInt32 is sufficient for the type of the Size property after all.

This is because the binary file header stores file sizes in 4 bytes.

In BPatch, file sizes are stored in LongInt = Int32 values, which limits the file size that can be handled to MaxInt. The choice of Int32 instead if UInt32 halves the maximum file size.

❓ Should BDiff and BPatch refuse to handle files larger than MaxInt bytes?

@delphidabbler
Copy link
Owner Author

To be consistent with BDiff,

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

should possibly have its DataSize parameter changed to NativeUInt, but given the file size limitation noted above, it probably doesn't matter.

Furthermore the type of the method's BFCheckSum parameter differs between the interface & implementation sections. Should be LongInt.

@delphidabbler
Copy link
Owner Author

All length related variables in

class procedure TPatcher.Apply(const SourceFileName, DestFileName: string);

are LongInt instead of Cardinal as would be expected in BDiff. This is true of the original C code too. Given the size restrictions imposed by the binary diff header, this is OK, but ❗could with commenting an explanation❗

@delphidabbler
Copy link
Owner Author

Furthermore the type of the method's BFCheckSum parameter differs between the interface & implementation sections. Should be LongInt.

There's a similar inconsistency with the SourceFileHandle parameter of

class procedure CopyData(const SourceFileHandle, DestFileHandle: Integer;

@delphidabbler
Copy link
Owner Author

delphidabbler commented May 20, 2023

Consideration of Delphi XE now irrelevant since updated to Delphi 11. (Issue #12)

@delphidabbler
Copy link
Owner Author

Some uses of Cardinal in place of size_t get coerced to Longint a.k.a. Int32 when stored, so it would seem more sensible to change Cardinal to Int32 in these cases.

I'm choosing Int32 in preference to Longint or Integer since it expresses the intention better.

@delphidabbler delphidabbler changed the title Replace use of Cardinal with NativeUInt where necessary Replace use of Cardinal with ~~NativeUInt ~~ other types where necessary May 29, 2023
@delphidabbler delphidabbler changed the title Replace use of Cardinal with ~~NativeUInt ~~ other types where necessary Replace use of Cardinal with other types where necessary May 30, 2023
@delphidabbler
Copy link
Owner Author

Some uses of Cardinal in place of size_t get coerced to Longint a.k.a. Int32 when stored, so it would seem more sensible to change Cardinal to Int32 in these cases.

This is the approach I'm taking. Reviewing all used of Cardinal and deciding to replace with either Int32, NativeUInt or THandle as relevant.

@delphidabbler delphidabbler added accepted and removed considering Under consideration labels May 30, 2023
@delphidabbler delphidabbler added this to the Next Release milestone May 30, 2023
@delphidabbler
Copy link
Owner Author

❓ Should BDiff and BPatch refuse to handle files larger than MaxInt bytes?

File size limited to 10MiB which can be overridden (see issue #22), will have absolute limit of MaxInt (see issue #32).

@delphidabbler
Copy link
Owner Author

Implemented by merge commit aa57c8d

@delphidabbler delphidabbler added completed Work completed and merged into develop on remote. Will close when merged to master and removed accepted labels Jun 1, 2023
@delphidabbler delphidabbler removed this from the Next Release milestone Jun 2, 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