ronantakizawa/codereview-bench huggingface dataset:
Repository: LadybirdBrowser/ladybird
File: Libraries/LibRegex/RegexByteCode.cpp (C++)

Code before:
/*
 * Copyright (c) 2020, Emanuel Sprung <emanuel.sprung@gmail.com>
 *
 * SPDX-License-Identifier: BSD-2-Clause
 */

#include "RegexByteCode.h"
#include "RegexDebug.h"

#include <AK/BinarySearch.h>
#include <AK/CharacterTypes.h>
#include <AK/StringBuilder.h>
#include <LibUnicode/CharacterTypes.h>

// U+2028 LINE SEPARATOR
constexpr static u32 const LineSeparator { 0x2028 };
// U+2029 PARAGRAPH SEPARATOR
constexpr static u32 const ParagraphSeparator { 0x2029 };

namespace regex {

static ALWAYS_INLINE AllOptions get_effective_options(MatchInput const& input, MatchState const& state)
{
    return state.current_options.value_or(input.regex_options);
}

template<typename ByteCode>
StringView OpCode<ByteCode>::name() const
{
    return name(opcode_id());
}

StringView execution_result_name(ExecutionResult result)
{
    switch (result) {
#define __ENUMERATE_EXECUTION_RESULT(x) \
    case ExecutionResult::x:            \
        return #x##sv;
        ENUMERATE_EXECUTION_RESULTS
#undef __ENUMERATE_EXECUTION_RESULT
    default:
        VERIFY_NOT_REACHED();
        return "<Unknown>"sv;
    }
}

StringView opcode_id_name(OpCodeId opcode)
{
    switch (opcode) {

Review comment (performance):
🤔 can we have `state.current_options` always present instead? this feels like it could make char matchers quite a bit slower as-is.

Code after:
/*
 * Copyright (c) 2020, Emanuel Sprung <emanuel.sprung@gmail.com>
 *
 * SPDX-License-Identifier: BSD-2-Clause
 */

#include "RegexByteCode.h"
#include "RegexDebug.h"

#include <AK/BinarySearch.h>
#include <AK/CharacterTypes.h>
#include <AK/StringBuilder.h>
#include <LibUnicode/CharacterTypes.h>

// U+2028 LINE SEPARATOR
constexpr static u32 const LineSeparator { 0x2028 };
// U+2029 PARAGRAPH SEPARATOR
constexpr static u32 const ParagraphSeparator { 0x2029 };

namespace regex {

template<typename ByteCode>
StringView OpCode<ByteCode>::name() const
{
    return name(opcode_id());
}

StringView execution_result_name(ExecutionResult result)
{
    switch (result) {
#define __ENUMERATE_EXECUTION_RESULT(x) \
    case ExecutionResult::x:            \
        return #x##sv;
        ENUMERATE_EXECUTION_RESULTS
#undef __ENUMERATE_EXECUTION_RESULT
    default:
        VERIFY_NOT_REACHED();
        return "<Unknown>"sv;
    }
}

StringView opcode_id_name(OpCodeId opcode)
{
    switch (opcode) {
#define __ENUMERATE_OPCODE(x) \
    case OpCodeId::x:         \
        return #x##sv;

        ENUMERATE_OPCODES
Repository: LadybirdBrowser/ladybird
File: Libraries/LibUnicode/TimeZone.cpp (C++)

Code before:

        switch (options.transition_rule) {
        case TimeZoneTransition::Options::TransitionRule::AnyTransition: {
            return TimeZoneTransition {
                .transition = AK::Duration::from_milliseconds(time_result),
            };
        }
        case TimeZoneTransition::Options::TransitionRule::TransitionWhereUTCOffsetChanges: {
            auto const* from_rule = result.getFrom();
            auto const* to_rule = result.getTo();

            i32 from_utc_offset = from_rule->getRawOffset() + from_rule->getDSTSavings();
            i32 to_utc_offset = to_rule->getRawOffset() + to_rule->getDSTSavings();

            if (from_utc_offset != to_utc_offset) {
                return TimeZoneTransition {
                    .transition = AK::Duration::from_milliseconds(time_result),
                };
            }

            current_icu_time = time_result;
            include_current_time = false;
            break;
        }
        default: {
            VERIFY_NOT_REACHED();
        }
        }
    }

    return OptionalNone {};
}

}


Review comment (suggestion):
Let's leave this off - then we'll get a compile error if a new transition rule is added but neglected to be handled

Code after:

        switch (options.transition_rule) {
        case TimeZoneTransition::Options::TransitionRule::AnyTransition: {
            return TimeZoneTransition {
                .transition = AK::Duration::from_milliseconds(time_result),
            };
        }
        case TimeZoneTransition::Options::TransitionRule::TransitionWhereUTCOffsetChanges: {
            auto const* from_rule = result.getFrom();
            auto const* to_rule = result.getTo();

            i32 from_utc_offset = from_rule->getRawOffset() + from_rule->getDSTSavings();
            i32 to_utc_offset = to_rule->getRawOffset() + to_rule->getDSTSavings();

            if (from_utc_offset != to_utc_offset) {
                return TimeZoneTransition {
                    .transition = AK::Duration::from_milliseconds(time_result),
                };
            }

            current_icu_time = time_result;
            include_current_time = false;
            break;
        }
        }
    }

    return OptionalNone {};
}

}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibCrypto/Hash/SHAKE.cpp (C++)

Code before:
 */

#include <AK/ByteBuffer.h>
#include <LibCrypto/Hash/SHAKE.h>
#include <LibCrypto/OpenSSL.h>
#include <openssl/evp.h>

namespace Crypto::Hash {

SHAKE::SHAKE(SHAKEKind kind)
{
    m_md = (kind == SHAKEKind::CSHAKE128) ? EVP_shake128() : EVP_shake256();
}

ErrorOr<ByteBuffer> SHAKE::digest(
    ReadonlyBytes data,
    u32 length,
    Optional<ReadonlyBytes> customization,
    Optional<ReadonlyBytes> function_name) const
{
    bool wants_cshake = (customization.has_value() && !customization->is_empty()) || (function_name.has_value() && !function_name->is_empty());

    if (wants_cshake) {
        // FIXME: implements cSHAKE with non-empty N or S
        return Error::from_string_literal(
            "cSHAKE with non-empty N or S is not supported yet "
            "(OpenSSL EVP limitation)");
    }

    if (length % 8 != 0) {
        return Error::from_string_literal(
            "SHAKE output length must be a multiple of 8 bits");
    }

    size_t output_bytes = length / 8;
    auto buf = TRY(ByteBuffer::create_uninitialized(output_bytes));

    auto ctx = TRY(OpenSSL_MD_CTX::wrap(EVP_MD_CTX_new()));

    OPENSSL_TRY(EVP_DigestInit_ex(ctx.ptr(), m_md, nullptr));

    OPENSSL_TRY(EVP_DigestUpdate(ctx.ptr(), data.data(), data.size()));

    OPENSSL_TRY(EVP_DigestFinalXOF(ctx.ptr(), buf.data(), output_bytes));

    return buf;
}

}


Review comment (suggestion):
```c++
if (wants_cshake) {
    // FIXME: Implement cSHAKE with non-empty N or S
    return Error::from_string_literal("cSHAKE with non-empty N or S is not supported yet (OpenSSL EVP limitation)");
}
```

etc.

Code after:
 */

#include <AK/ByteBuffer.h>
#include <LibCrypto/Hash/SHAKE.h>
#include <LibCrypto/OpenSSL.h>
#include <openssl/evp.h>

namespace Crypto::Hash {

SHAKE::SHAKE(SHAKEKind kind)
{
    m_md = (kind == SHAKEKind::CSHAKE128) ? EVP_shake128() : EVP_shake256();
}

ErrorOr<ByteBuffer> SHAKE::digest(
    ReadonlyBytes data,
    u32 length,
    Optional<ReadonlyBytes> customization,
    Optional<ReadonlyBytes> function_name) const
{
    bool wants_cshake = (customization.has_value() && !customization->is_empty()) || (function_name.has_value() && !function_name->is_empty());

    if (wants_cshake) {
        // FIXME: Implement cSHAKE with non-empty N or S
        return Error::from_string_literal("cSHAKE with non-empty N or S is not supported yet (OpenSSL EVP limitation)");
    }

    if (length % 8 != 0) {
        return Error::from_string_literal("SHAKE output length must be a multiple of 8 bits");
    }

    size_t output_bytes = length / 8;
    auto buf = TRY(ByteBuffer::create_uninitialized(output_bytes));

    auto ctx = TRY(OpenSSL_MD_CTX::wrap(EVP_MD_CTX_new()));

    OPENSSL_TRY(EVP_DigestInit_ex(ctx.ptr(), m_md, nullptr));

    OPENSSL_TRY(EVP_DigestUpdate(ctx.ptr(), data.data(), data.size()));

    OPENSSL_TRY(EVP_DigestFinalXOF(ctx.ptr(), buf.data(), output_bytes));

    return buf;
}

}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibCore/Proxy.h (C/C++)

Code before:
/*
 * Copyright (c) 2022, Ali Mohammad Pur <mpfard@serenityos.org>
 *
 * SPDX-License-Identifier: BSD-2-Clause
 */

#pragma once

#include <AK/Error.h>
#include <AK/IPv4Address.h>
#include <AK/Types.h>
#include <LibURL/URL.h>

namespace Core {

// FIXME: Username/password support.
struct ProxyData {
    enum Type {
        Direct,
        SOCKS5,
        __Count,
    } type { Type::Direct };

    IPv4Address host_ipv4;
    int port { 0 };

    bool operator==(ProxyData const& other) const = default;

    static ErrorOr<ProxyData> parse_url(URL::URL const& url)
    {
        ProxyData proxy_data;
        if (url.scheme() != "socks5")
            return Error::from_string_literal("Unsupported proxy type");

        proxy_data.type = ProxyData::Type::SOCKS5;

        if (!url.host().has_value() || !url.host()->has<IPv4Address>())
            return Error::from_string_literal("Invalid proxy host, must be an IPv4 address");
        proxy_data.host_ipv4 = url.host()->get<IPv4Address>();

        auto port = url.port();
        if (!port.has_value())
            return Error::from_string_literal("Invalid proxy, must have a port");
        proxy_data.port = *port;

        return proxy_data;

Review comment (suggestion):
I've wondered about bringing in Magic Enum for stuff like this:
```c++
enum class Color {
    Red,
    Blue,
    Green,
    Orange,
};

constexpr auto count = magic_enum::enum_count<Color>(); // 4
```

(We could also implement the few things we might need, seems like it's a little bit of template magic using index sequences)

But using that would let us avoid touching every enum and switch statement

Code after:
/*
 * Copyright (c) 2022, Ali Mohammad Pur <mpfard@serenityos.org>
 *
 * SPDX-License-Identifier: BSD-2-Clause
 */

#pragma once

#include <AK/Error.h>
#include <AK/IPv4Address.h>
#include <AK/Types.h>
#include <LibURL/URL.h>

namespace Core {

// FIXME: Username/password support.
struct ProxyData {
    enum Type {
        Direct,
        SOCKS5,
    } type { Type::Direct };

    IPv4Address host_ipv4;
    u16 port { 0 };

    bool operator==(ProxyData const& other) const = default;

    static ErrorOr<ProxyData> parse_url(URL::URL const& url)
    {
        ProxyData proxy_data;
        if (url.scheme() != "socks5")
            return Error::from_string_literal("Unsupported proxy type");

        proxy_data.type = ProxyData::Type::SOCKS5;

        if (!url.host().has_value() || !url.host()->has<IPv4Address>())
            return Error::from_string_literal("Invalid proxy host, must be an IPv4 address");
        proxy_data.host_ipv4 = url.host()->get<IPv4Address>();

        auto port = url.port();
        if (!port.has_value())
            return Error::from_string_literal("Invalid proxy, must have a port");
        proxy_data.port = *port;

        return proxy_data;
    }
Repository: LadybirdBrowser/ladybird
File: Libraries/LibWeb/CredentialManagement/FederatedCredential.cpp (C++)

Code before:
#include <LibWeb/Bindings/Intrinsics.h>
#include <LibWeb/CredentialManagement/FederatedCredential.h>
#include <LibWeb/CredentialManagement/FederatedCredentialOperations.h>

namespace Web::CredentialManagement {

GC_DEFINE_ALLOCATOR(FederatedCredential);

// https://www.w3.org/TR/credential-management-1/#dom-federatedcredential-federatedcredential
WebIDL::ExceptionOr<GC::Ref<FederatedCredential>> FederatedCredential::construct_impl(JS::Realm& realm, FederatedCredentialInit const& data)
{
    // 1. Let r be the result of executing Create a FederatedCredential from FederatedCredentialInit on data. If that
    // threw an exception, rethrow that exception.
    // 2. Return r.
    return create_federated_credential(realm, data);
}

FederatedCredential::~FederatedCredential()
{
}

FederatedCredential::FederatedCredential(JS::Realm& realm, FederatedCredentialInit const& init, URL::Origin const& origin)
    : Credential(realm, init.id)
    , CredentialUserData(init.name.value_or(String {}), init.icon_url.value_or(String {}))
    , m_provider(init.provider)
    , m_origin(origin)
{
}

void FederatedCredential::initialize(JS::Realm& realm)
{
    WEB_SET_PROTOTYPE_FOR_INTERFACE(FederatedCredential);
    Base::initialize(realm);
}

}


Review comment (security):
I don't see where we were making copies before. We were correctly moving the origins created in `PasswordCredential::construct_impl` into this constructor.

Instead, it looks like we now actually force a copy here and in the `PasswordCredential` constructor. So unless I'm missing something, this patch has increased the number of copies.

Code after:
#include <LibWeb/Bindings/Intrinsics.h>
#include <LibWeb/CredentialManagement/FederatedCredential.h>
#include <LibWeb/CredentialManagement/FederatedCredentialOperations.h>

namespace Web::CredentialManagement {

GC_DEFINE_ALLOCATOR(FederatedCredential);

// https://www.w3.org/TR/credential-management-1/#dom-federatedcredential-federatedcredential
WebIDL::ExceptionOr<GC::Ref<FederatedCredential>> FederatedCredential::construct_impl(JS::Realm& realm, FederatedCredentialInit const& data)
{
    // 1. Let r be the result of executing Create a FederatedCredential from FederatedCredentialInit on data. If that
    // threw an exception, rethrow that exception.
    // 2. Return r.
    return create_federated_credential(realm, data);
}

FederatedCredential::~FederatedCredential()
{
}

FederatedCredential::FederatedCredential(JS::Realm& realm, FederatedCredentialInit const& init, URL::Origin origin)
    : Credential(realm, init.id)
    , CredentialUserData(init.name.value_or(String {}), init.icon_url.value_or(String {}))
    , m_provider(init.provider)
    , m_origin(move(origin))
{
}

void FederatedCredential::initialize(JS::Realm& realm)
{
    WEB_SET_PROTOTYPE_FOR_INTERFACE(FederatedCredential);
    Base::initialize(realm);
}

}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibWeb/CredentialManagement/PasswordCredentialOperations.h (C/C++)

Code before:
/*
 * Copyright (c) 2025, Kenneth Myhra <kennethmyhra@serenityos.org>
 *
 * SPDX-License-Identifier: BSD-2-Clause
 */

#pragma once

#include <LibWeb/CredentialManagement/PasswordCredential.h>

namespace Web::CredentialManagement {

// https://www.w3.org/TR/credential-management-1/#abstract-opdef-create-a-passwordcredential-from-an-htmlformelement
WebIDL::ExceptionOr<GC::Ref<PasswordCredential>> create_password_credential(JS::Realm& realm, GC::Ptr<HTML::HTMLFormElement> const&, URL::Origin const&);

// https://www.w3.org/TR/credential-management-1/#abstract-opdef-create-a-passwordcredential-from-passwordcredentialdata
WebIDL::ExceptionOr<GC::Ref<PasswordCredential>> create_password_credential(JS::Realm& realm, PasswordCredentialData const&, URL::Origin const&);

}


Review comment (suggestion):
Incidentally, `GC::Ptr` should not be passed by reference either. It's just a pointer.

(It also looks like we are assuming the pointer is non-null. We use `GC::Ref` for such cases.)

Code after:
/*
 * Copyright (c) 2025, Kenneth Myhra <kennethmyhra@serenityos.org>
 *
 * SPDX-License-Identifier: BSD-2-Clause
 */

#pragma once

#include <LibWeb/CredentialManagement/PasswordCredential.h>

namespace Web::CredentialManagement {

WebIDL::ExceptionOr<GC::Ref<PasswordCredential>> create_password_credential(JS::Realm& realm, GC::Ref<HTML::HTMLFormElement>, URL::Origin);
WebIDL::ExceptionOr<GC::Ref<PasswordCredential>> create_password_credential(JS::Realm& realm, PasswordCredentialData const&, URL::Origin);

}
Repository: LadybirdBrowser/ladybird
File: Libraries/LibGC/Weak.h (C/C++)

Code before:
        return *ptr();
    }

    Ptr<T> ptr() const { return static_cast<T*>(impl().ptr()); }

    explicit operator bool() const { return !!ptr(); }
    bool operator!() const { return !ptr(); }

    operator T*() const { return ptr(); }

    Ref<T> as_nonnull() const
    {
        ASSERT(ptr());
        return *ptr();
    }

    WeakImpl& impl() const { return *m_impl; }

private:
    NonnullRefPtr<WeakImpl> m_impl { WeakImpl::the_null_weak_impl };
};

template<typename T, typename Callback>
auto weak_callback(T& obj, Callback&& callback)
{
    return [weak = Weak<T> { obj }, cb = forward<Callback>(callback)](auto&&... args) mutable {
        if (weak)
            cb(*weak, forward<decltype(args)>(args)...);
    };
}

template<typename T, typename U>
inline bool operator==(Weak<T> const& a, Ptr<U> const& b)
{
    return a.ptr() == b.ptr();
}

template<typename T, typename U>
inline bool operator==(Weak<T> const& a, Ref<U> const& b)
{
    return a.ptr() == b.ptr();
}

}

namespace AK {

template<typename T>
struct Formatter<GC::Weak<T>> : Formatter<T const*> {
    ErrorOr<void> format(FormatBuilder& builder, GC::Weak<T> const& value)
    {

Review comment (suggestion):
Maybe add a comment that, unlike `GC::Function`, captures are not safe from GC. The usage here is all fine ofc, but I can imagine someone grabbing this in the future thinking it protects captures the same way as `GC::Function`.

Code after:
        return *ptr();
    }

    Ptr<T> ptr() const { return static_cast<T*>(impl().ptr()); }

    explicit operator bool() const { return !!ptr(); }
    bool operator!() const { return !ptr(); }

    operator T*() const { return ptr(); }

    Ref<T> as_nonnull() const
    {
        ASSERT(ptr());
        return *ptr();
    }

    WeakImpl& impl() const { return *m_impl; }

private:
    NonnullRefPtr<WeakImpl> m_impl { WeakImpl::the_null_weak_impl };
};

// NOTE: Unlike GC::Function, captures in this callback are not visited by the garbage collector.
//       Do not capture GC-managed objects without ensuring they are kept alive through other means.
template<typename T, typename Callback>
auto weak_callback(T& obj, Callback&& callback)
{
    return [weak = Weak<T> { obj }, cb = forward<Callback>(callback)](auto&&... args) mutable {
        if (weak)
            cb(*weak, forward<decltype(args)>(args)...);
    };
}

template<typename T, typename U>
inline bool operator==(Weak<T> const& a, Ptr<U> const& b)
{
    return a.ptr() == b.ptr();
}

template<typename T, typename U>
inline bool operator==(Weak<T> const& a, Ref<U> const& b)
{
    return a.ptr() == b.ptr();
}

}

namespace AK {

template<typename T>
struct Formatter<GC::Weak<T>> : Formatter<T const*> {
Repository: Lightning-AI/pytorch-lightning
File: pytorch_lightning/callbacks/gpu_stats_monitor.py (Python)

Code before:
        if self._log_stats.memory_utilization:
            stat_keys.extend([('memory.used', 'MB'), ('memory.free', 'MB'), ('utilization.memory', '%')])

        return stat_keys

    def _get_gpu_device_stat_keys(self) -> List[Tuple[str, str]]:
        """Get the device stats keys"""
        stat_keys = []

        if self._log_stats.fan_speed:
            stat_keys.append(('fan.speed', '%'))

        if self._log_stats.temperature:
            stat_keys.extend([('temperature.gpu', '°C'), ('temperature.memory', '°C')])

        return stat_keys

    def _should_log(self, trainer) -> bool:
        should_log = (
            (trainer.global_step + 1) % trainer.log_every_n_steps == 0
            or trainer.should_stop
            or trainer.fast_dev_run
        )

        return should_log


Review comment (suggestion):
```suggestion
        should_log = (
            (trainer.global_step + 1) % trainer.log_every_n_steps == 0
            or trainer.should_stop
        )

        should_log = should_log and trainer.fast_dev_run
        return should_log
```

Code after:
        if self._log_stats.memory_utilization:
            stat_keys.extend([('memory.used', 'MB'), ('memory.free', 'MB'), ('utilization.memory', '%')])

        return stat_keys

    def _get_gpu_device_stat_keys(self) -> List[Tuple[str, str]]:
        """Get the device stats keys"""
        stat_keys = []

        if self._log_stats.fan_speed:
            stat_keys.append(('fan.speed', '%'))

        if self._log_stats.temperature:
            stat_keys.extend([('temperature.gpu', '°C'), ('temperature.memory', '°C')])

        return stat_keys

    @staticmethod
    def _should_log(trainer) -> bool:
        should_log = (
            (trainer.global_step + 1) % trainer.log_every_n_steps == 0
            or trainer.should_stop
        )

        should_log = should_log and not trainer.fast_dev_run
        return should_log
Repository: Lightning-AI/pytorch-lightning
File: pytorch_lightning/callbacks/lr_monitor.py (Python)

Code before:
                        break
                    i, name = i + 1, f'{opt_name}-{i}'

            # Multiple param groups for the same schduler
            param_groups = sch.optimizer.param_groups

            if len(param_groups) != 1:
                for i, pg in enumerate(param_groups):
                    temp = f'{name}/pg{i + 1}'
                    names.append(temp)
            else:
                names.append(name)

            self.lr_sch_names.append(name)

        return names

    def _should_log(self, trainer) -> bool:
        should_log = (
            (trainer.global_step + 1) % trainer.log_every_n_steps == 0
            or trainer.should_stop
            or trainer.fast_dev_run
        )

        return should_log


Review comment (suggestion):
```suggestion
        )

        should_log = should_log and trainer.fast_dev_run
        return should_log
```

Code after:
                        break
                    i, name = i + 1, f'{opt_name}-{i}'

            # Multiple param groups for the same schduler
            param_groups = sch.optimizer.param_groups

            if len(param_groups) != 1:
                for i, pg in enumerate(param_groups):
                    temp = f'{name}/pg{i + 1}'
                    names.append(temp)
            else:
                names.append(name)

            self.lr_sch_names.append(name)

        return names

    @staticmethod
    def _should_log(trainer) -> bool:
        should_log = (
            (trainer.global_step + 1) % trainer.log_every_n_steps == 0
            or trainer.should_stop
        )

        should_log = should_log and not trainer.fast_dev_run
        return should_log
Repository: Lightning-AI/pytorch-lightning
File: pytorch_lightning/callbacks/gpu_stats_monitor.py (Python)

Code before:
            stat_keys.append(('utilization.gpu', '%'))

        if self._log_stats.memory_utilization:
            stat_keys.extend([('memory.used', 'MB'), ('memory.free', 'MB'), ('utilization.memory', '%')])

        return stat_keys

    def _get_gpu_device_stat_keys(self) -> List[Tuple[str, str]]:
        """Get the device stats keys"""
        stat_keys = []

        if self._log_stats.fan_speed:
            stat_keys.append(('fan.speed', '%'))

        if self._log_stats.temperature:
            stat_keys.extend([('temperature.gpu', '°C'), ('temperature.memory', '°C')])

        return stat_keys

    def _should_log(self, trainer) -> bool:
        should_log = (
            (trainer.global_step + 1) % trainer.log_every_n_steps == 0
            or trainer.should_stop
        )

        should_log = should_log and trainer.fast_dev_run
        return should_log


Review comment (suggestion):
```suggestion
        should_log = should_log and not trainer.fast_dev_run
```

Code after:
            stat_keys.append(('utilization.gpu', '%'))

        if self._log_stats.memory_utilization:
            stat_keys.extend([('memory.used', 'MB'), ('memory.free', 'MB'), ('utilization.memory', '%')])

        return stat_keys

    def _get_gpu_device_stat_keys(self) -> List[Tuple[str, str]]:
        """Get the device stats keys"""
        stat_keys = []

        if self._log_stats.fan_speed:
            stat_keys.append(('fan.speed', '%'))

        if self._log_stats.temperature:
            stat_keys.extend([('temperature.gpu', '°C'), ('temperature.memory', '°C')])

        return stat_keys

    @staticmethod
    def _should_log(trainer) -> bool:
        should_log = (
            (trainer.global_step + 1) % trainer.log_every_n_steps == 0
            or trainer.should_stop
        )

        should_log = should_log and not trainer.fast_dev_run
        return should_log