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

Do not use FillChar to initialize records #150

Open
3 tasks done
Chadehoc opened this issue Dec 29, 2023 · 3 comments
Open
3 tasks done

Do not use FillChar to initialize records #150

Chadehoc opened this issue Dec 29, 2023 · 3 comments
Labels
feature New feature or request rule Improvements or additions to rules

Comments

@Chadehoc
Copy link

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

NoFillCharOnRecords

Rule description

Using FillChar or ZeroMemory to initialize a record can evolve badly if one adds a managed field to the record. If the Delphi version supports it, it is suggested to use := Default(T) instead.

This was suggested as part of #149, but is too dependent on using a recent Delphi version. Making it a separate rule is more adequate.

Rationale

Using FillChar on a managed field can skip a refcount Release and result in large memory leaks.

@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 29, 2023
@Cirras
Copy link
Collaborator

Cirras commented Dec 29, 2023

Thanks for the suggestion!

Agreed that this is a very appropriate rule for new Delphi versions, where the Default intrinsic is unamibiguously the most correct tool for the job of default-initializing any variable.

I also like that you provided an example of why this code smell can lead to real-world bugs. 👍

@Cirras Cirras removed the triage This needs to be triaged by a maintainer label Dec 29, 2023
@sglienke
Copy link

FWIW using FillChar to initialize a record before use is not an issue because the compiler would have inserted appropriate code to initialize any managed fields into the prologue of the routine - the issue is using FillChar to clear an already used record that might contain some data in its managed fields.

@Cirras
Copy link
Collaborator

Cirras commented Jan 10, 2024

FWIW using FillChar to initialize a record before use is not an issue because the compiler would have inserted appropriate code to initialize any managed fields into the prologue of the routine - the issue is using FillChar to clear an already used record that might contain some data in its managed fields.

Thanks for the additional information, Stefan.
I think we should mention that subtlety of the behavior when writing the rule description.

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

3 participants