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

Make 'logger' a std::unique_ptr #6962

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

edolstra
Copy link
Member

This prevents it from being leaked (see bb411e4 for an example of this).

This also fixes a bug in the ProgressBar destructor when isTTY = false.

This prevents it from being leaked (see
bb411e4 for an example of this).
@@ -110,7 +105,7 @@ namespace nix {
CaptureLogging l;
auto v = eval("builtins.trace \"test string 123\" 123");
ASSERT_THAT(v, IsIntEq(123));
auto text = l.get();
auto text = ((CaptureLogger *) logger.get())->get();
Copy link
Member

Choose a reason for hiding this comment

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

What's going on here?

@Ericson2314 Ericson2314 added the contributor-experience Developer experience for Nix contributors label Mar 3, 2023
@fricklerhandwerk
Copy link
Contributor

Triaged in the Nix team meeting:

  • @thufschmitt: attempted this multiple times but CI failed for various reasons
    • the bug it would fix is insignificant, not worth the efford right now
  • postponed

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-03-03-nix-team-meeting-minutes-37/25998/1

@fricklerhandwerk
Copy link
Contributor

@edolstra do you want to take another look at it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug contributor-experience Developer experience for Nix contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants