Skip to content

Fix virtual method calls during construction #13493

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 17, 2025
Merged

Fix virtual method calls during construction #13493

merged 2 commits into from
Jul 17, 2025

Conversation

Mic92
Copy link
Member

@Mic92 Mic92 commented Jul 17, 2025

Summary

Fix clang-tidy warnings about calling virtual methods during object construction in:

  • S3BinaryCacheStoreImpl: Move getUri() calls from constructor to methods
  • LocalBinaryCacheStore: Move getBinaryCacheDir() call from initializer list to constructor body

Calling virtual methods in constructors is undefined behavior in C++ as the vtable may not be fully initialized yet.

Mic92 added 2 commits July 17, 2025 15:45
@Mic92
Fix virtual method calls during construction in S3BinaryCacheStoreImpl
f12f96b 
Move init() call from constructor to openStore() method to avoid calling
virtual methods during object construction. This prevents undefined
behavior when virtual methods are called before the object is fully
constructed.
@Mic92
Fix virtual method calls during construction in LocalBinaryCacheStore
44963da 
Move init() call from constructor to openStore() method to avoid calling
virtual methods during object construction. This prevents undefined
behavior when virtual methods are called before the object is fully
constructed.
@Mic92 Mic92 requested a review from Ericson2314 as a code owner July 17, 2025 13:46
@github-actions github-actions bot added the store Issues and pull requests concerning the Nix store label Jul 17, 2025
edolstra
edolstra approved these changes Jul 17, 2025
View reviewed changes
Copy link
Member

@edolstra edolstra left a comment

Choose a reason for hiding this comment

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

We could also move the various init() methods into the constructors of their corresponding classes.

BTW, it looks like HttpBinaryCache::init() is currently not called anywhere?

@Mic92 Mic92 merged commit 4c95086 into NixOS:master Jul 17, 2025
12 checks passed
@Mic92 Mic92 deleted the clang-tidy-virtual-methods branch July 17, 2025 15:41
@roberth roberth mentioned this pull request Aug 21, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edolstra edolstra edolstra approved these changes

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314 Ericson2314 is a code owner

Assignees
No one assigned
Labels
store Issues and pull requests concerning the Nix store
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Mic92 @edolstra