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

Record memory should be made deterministic in constructors #149

Open
3 tasks done
Chadehoc opened this issue Dec 22, 2023 · 1 comment
Open
3 tasks done

Record memory should be made deterministic in constructors #149

Chadehoc opened this issue Dec 22, 2023 · 1 comment
Labels
feature New feature or request rule Improvements or additions to rules

Comments

@Chadehoc
Copy link

Chadehoc commented Dec 22, 2023

Prerequisites

  • This rule has not already been suggested.
  • This should be a new rule, not an improvement to an existing rule.
  • This rule would be generally useful, not specific to my code or setup.

Suggested rule title

DeterministicRecordMemory

Rule description

This had been suggested as part of #131 but was rather made a separate rule.

Generally, a record can be initialized by assigning all its fields (accepted for rule #131). But there is one case for which this is not enough.

The default hasher for records is wrong when there are "holes" due to field alignment in non-packed records, for example an Integer and an Int64 field. The default hasher uses also the memory values within the holes, which violates the expected invariant that two values that are Equal should have the same hash.

This rule requires a stricter initialization method for record types that are not packed and have alignment "holes". Then the only proper initialization should be done by assigning the full record, to Default(T) for example, or to another (initialized) value.

It was suggested that erasing memory using FillChar or ZeroMemory could be ok, but I argue that using those to initialize a record is bad practice (because it can evolve badly), and is always better done with := Default(T).

Thanks to #127, this rule can be restricted to constructors and Initialize, as is #131.

Rationale

Using such records as dictionary keys, for example, can lead to nasty bugs if one only ensures that all fields are initialized. The unused memory should be zeroed to get consistent hashes.

@Chadehoc Chadehoc added feature New feature or request rule Improvements or additions to rules triage This needs to be triaged by a maintainer labels Dec 22, 2023
@Cirras
Copy link
Collaborator

Cirras commented Dec 23, 2023

Thanks, I think this rule is worth having.
Regarding Default vs FillChar vs ZeroMemory, something to bear in mind is that SonarDelphi supports many versions of Delphi (including versions that predate the Default intrinsic).

@Cirras Cirras removed the triage This needs to be triaged by a maintainer label Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request rule Improvements or additions to rules
Projects
None yet
Development

No branches or pull requests

2 participants