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 embed Record interface into baseRecord #398

Conversation

antoninbas
Copy link
Member

As far as I know this is an anti-pattern, and I am not sure why it was done this way. baseRecord is just here to help reduce code duplication in dataRecord and templateRecord, which both implement the Record interface.

When embedding Recod in baseRecord, we 1) obfuscate things, making it harder to detect at compile time that one of the record types is missing a method implementation, and 2) increase the size of the baseRecord struct needlessly (after this change the size is reduced from 88B down to 72B on my machine).

As far as I know this is an anti-pattern, and I am not sure why it was
done this way. baseRecord is just here to help reduce code duplication
in dataRecord and templateRecord, which both implement the Record
interface.

When embedding Recod in baseRecord, we 1) obfuscate things, making it
harder to detect at compile time that one of the record types is missing
a method implementation, and 2) increase the size of the baseRecord
struct needlessly (after this change the size is reduced from 88B down
to 72B on my machine).

Signed-off-by: Antonin Bas <[email protected]>
Copy link
Contributor

@yuntanghsu yuntanghsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@antoninbas antoninbas merged commit 8ded9f6 into vmware:main Jan 13, 2025
8 checks passed
@antoninbas antoninbas deleted the do-not-embed-Record-interface-into-baseRecord branch January 13, 2025 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants