ronantakizawa/codereview-bench huggingface dataset:
Repository: SerenityOS/serenity
File: Kernel/Devices/GPU/VirtIO/Console.cpp (C++)

Code before:
/*
 * Copyright (c) 2021, Sahan Fernando <sahan.h.fernando@gmail.com>
 *
 * SPDX-License-Identifier: BSD-2-Clause
 */

#include <Kernel/Arch/Delay.h>
#include <Kernel/Devices/GPU/VirtIO/Console.h>
#include <Kernel/Devices/TTY/VirtualConsole.h>
#include <Kernel/Tasks/WorkQueue.h>

namespace Kernel::Graphics::VirtIOGPU {

constexpr static AK::Duration refresh_interval = AK::Duration::from_milliseconds(16);

NonnullLockRefPtr<Console> Console::initialize(VirtIODisplayConnector& parent_display_connector)
{
    auto current_resolution = parent_display_connector.current_mode_setting();
    return adopt_lock_ref(*new Console(parent_display_connector, current_resolution));
}

Console::Console(VirtIODisplayConnector const& parent_display_connector, DisplayConnector::ModeSetting current_resolution)
    : GenericFramebufferConsole(current_resolution.horizontal_active, current_resolution.vertical_active, current_resolution.horizontal_stride)
    , m_parent_display_connector(parent_display_connector)
{
    // NOTE: Clear the framebuffer, in case it's left with some garbage.
    memset(framebuffer_data(), 0, current_resolution.horizontal_stride * current_resolution.vertical_active);

    auto [process, _] = Process::create_kernel_process("Console Refresh Task"sv, [this]() { refresh_task(); }).release_value_but_fixme_should_propagate_errors();
    m_refresh_process = move(process);
}


Review comment (suggestion):
This header is unused. You don't use `microseconds_delay()`.

Code after:
/*
 * Copyright (c) 2021, Sahan Fernando <sahan.h.fernando@gmail.com>
 *
 * SPDX-License-Identifier: BSD-2-Clause
 */

#include <Kernel/Devices/GPU/VirtIO/Console.h>
#include <Kernel/Devices/TTY/VirtualConsole.h>
#include <Kernel/Tasks/WorkQueue.h>

namespace Kernel::Graphics::VirtIOGPU {

constexpr static AK::Duration refresh_interval = AK::Duration::from_milliseconds(16);

NonnullLockRefPtr<Console> Console::initialize(VirtIODisplayConnector& parent_display_connector)
{
    auto current_resolution = parent_display_connector.current_mode_setting();
    return adopt_lock_ref(*new Console(parent_display_connector, current_resolution));
}

Console::Console(VirtIODisplayConnector const& parent_display_connector, DisplayConnector::ModeSetting current_resolution)
    : GenericFramebufferConsole(current_resolution.horizontal_active, current_resolution.vertical_active, current_resolution.horizontal_stride)
    , m_parent_display_connector(parent_display_connector)
{
    // NOTE: Clear the framebuffer, in case it's left with some garbage.
    memset(framebuffer_data(), 0, current_resolution.horizontal_stride * current_resolution.vertical_active);

    auto [process, _] = Process::create_kernel_process("Console Refresh Task"sv, [this]() { refresh_task(); }).release_value_but_fixme_should_propagate_errors();
    m_refresh_process = move(process);
}

void Console::set_resolution(size_t width, size_t height, size_t pitch)
Repository: SerenityOS/serenity
File: Userland/Libraries/LibGfx/ImageFormats/JBIG2Loader.cpp (C++)

Code before:
                    return code.first_value.value() - value;
                return value + code.first_value.value();
            }
        }
    }
    VERIFY_NOT_REACHED();
}

ErrorOr<Optional<i32>> HuffmanTable::read_symbol(BigEndianInputBitStream& stream) const
{
    VERIFY(m_has_oob_symbol);
    return read_symbol_internal(stream);
}

ErrorOr<i32> HuffmanTable::read_symbol_non_oob(BigEndianInputBitStream& stream) const
{
    VERIFY(!m_has_oob_symbol);
    auto result = TRY(read_symbol_internal(stream));
    return result.value();
}

}

class Symbol : public RefCounted<Symbol> {
public:
    static NonnullRefPtr<Symbol> create(NonnullRefPtr<BilevelImage> bitmap)
    {
        return adopt_ref(*new Symbol(move(bitmap)));
    }

    BilevelImage const& bitmap() const { return *m_bitmap; }

private:
    Symbol(NonnullRefPtr<BilevelImage> bitmap)
        : m_bitmap(move(bitmap))
    {
    }

    NonnullRefPtr<BilevelImage> m_bitmap;
};

struct SegmentData {
    SegmentData(JBIG2::SegmentHeader header, ReadonlyBytes data)
        : header(header)
        , data(data)
    {
    }

    JBIG2::SegmentHeader header;
    ReadonlyBytes data;


Review comment (suggestion):
I'm very supportive of this commit; I almost made the change myself once.

Having said that, the point of `Symbol` is to be a RefCounted wrapper around BilevelImage, so after this commit this class is iirc pretty pointless.

But I can clean it up in a separate commit later on.

(No action needed.)

Code after:
                    return code.first_value.value() - value;
                return value + code.first_value.value();
            }
        }
    }
    VERIFY_NOT_REACHED();
}

ErrorOr<Optional<i32>> HuffmanTable::read_symbol(BigEndianInputBitStream& stream) const
{
    VERIFY(m_has_oob_symbol);
    return read_symbol_internal(stream);
}

ErrorOr<i32> HuffmanTable::read_symbol_non_oob(BigEndianInputBitStream& stream) const
{
    VERIFY(!m_has_oob_symbol);
    auto result = TRY(read_symbol_internal(stream));
    return result.value();
}

}

class Symbol : public RefCounted<Symbol> {
public:
    static NonnullRefPtr<Symbol> create(BilevelSubImage bitmap)
    {
        return adopt_ref(*new Symbol(move(bitmap)));
    }

    BilevelSubImage const& bitmap() const { return m_bitmap; }

private:
    Symbol(BilevelSubImage bitmap)
        : m_bitmap(move(bitmap))
    {
    }

    BilevelSubImage m_bitmap;
};

struct SegmentData {
    SegmentData(JBIG2::SegmentHeader header, ReadonlyBytes data)
        : header(header)
        , data(data)
    {
    }

    JBIG2::SegmentHeader header;
    ReadonlyBytes data;
Repository: SerenityOS/serenity
File: Userland/Libraries/LibGfx/Matrix.h (C/C++)

Code before:
    [[nodiscard]] constexpr Matrix inverse() const
    {
        return adjugate() / determinant();
    }

    [[nodiscard]] constexpr Matrix transpose() const
    {
        Matrix result;
        for (size_t i = 0; i < N; ++i) {
            for (size_t j = 0; j < N; ++j)
                result.m_elements[i][j] = m_elements[j][i];
        }
        return result;
    }

    [[nodiscard]] constexpr T sum() const
    {
        T s = 0;
        for (size_t i = 0; i < N; ++i) {
            for (size_t j = 0; j < N; ++j)
                s += m_elements[j][i];
        }
        return s;
    }

    [[nodiscard]] constexpr Matrix normalized() const
    {
        auto normalized = *this;
        auto sum_ = sum();

        for (size_t i = 0; i < N; ++i) {
            for (size_t j = 0; j < N; ++j)
                normalized.m_elements[j][i] /= sum_;
        }
        return normalized;
    }

    [[nodiscard]] constexpr Matrix hadamard_product(Matrix const& matrix)
    {
        Matrix product;
        for (size_t i = 0; i < N; ++i) {
            for (size_t j = 0; j < N; ++j)
                product.m_elements[i][j] = m_elements[i][j] * matrix.m_elements[i][j];
        }
        return product;
    }

    template<size_t U>
    [[nodiscard]] constexpr Matrix<U, T> submatrix_from_topleft() const
    requires(U > 0 && U < N)
    {

Review comment (suggestion):
This is even weirder. Normalizing a matrix isn't really a thing I believe. It can have several meanings, and none means "divide by the sum".

I'd write the implementation as `return *this / sum();` but as that's so little code I'd rather put that code in the caller, as I don't think it'll commonly be used.

Code after:
    [[nodiscard]] constexpr Matrix inverse() const
    {
        return adjugate() / determinant();
    }

    [[nodiscard]] constexpr Matrix transpose() const
    {
        Matrix result;
        for (size_t i = 0; i < N; ++i) {
            for (size_t j = 0; j < N; ++j)
                result.m_elements[i][j] = m_elements[j][i];
        }
        return result;
    }

    [[nodiscard]] constexpr T element_sum() const
    {
        T s = 0;
        for (size_t i = 0; i < N; ++i) {
            for (size_t j = 0; j < N; ++j)
                s += m_elements[i][j];
        }
        return s;
    }

    [[nodiscard]] constexpr Matrix hadamard_product(Matrix const& matrix)
    {
        Matrix product;
        for (size_t i = 0; i < N; ++i) {
            for (size_t j = 0; j < N; ++j)
                product.m_elements[i][j] = m_elements[i][j] * matrix.m_elements[i][j];
        }
        return product;
    }

    template<size_t U>
    [[nodiscard]] constexpr Matrix<U, T> submatrix_from_topleft() const
    requires(U > 0 && U < N)
    {
        Matrix<U, T> result;
        for (size_t i = 0; i < U; ++i) {
            for (size_t j = 0; j < U; ++j)
                result.m_elements[i][j] = m_elements[i][j];
        }
        return result;
    }

    constexpr bool is_invertible() const
    {
        return determinant() != static_cast<T>(0.0);
    }
Repository: SerenityOS/serenity
File: Userland/Libraries/LibGfx/TIFFGenerator.py (Python)

Code before:
            m_main_tags.set("Model"sv, model().value());
        if (make().has_value())
            m_main_tags.set("Manufacturer"sv, make().value());
        if (software().has_value())
            m_main_tags.set("Software"sv, software().value());
        if (date_time().has_value())
            m_main_tags.set("Creation Time"sv, date_time().value());
        if (artist().has_value())
            m_main_tags.set("Author"sv, artist().value());
    }}

    void add_entry(StringView key, Vector<TIFF::Value>&& value) {{
        m_data.set(key, move(value));
    }}

    HashMap<StringView, Vector<TIFF::Value>> m_data;
}};
"""

    return output


def generate_metadata_file(tags: List[Tag]) -> str:
    output = fR"""{LICENSE}

// Generated by TIFFGenerator.py. Do not edit.

#pragma once

#include <AK/HashMap.h>
#include <AK/Variant.h>
#include <AK/Vector.h>
#include <LibGfx/Size.h>
#include <LibGfx/ImageFormats/ImageDecoder.h>

namespace Gfx {{

class ExifMetadata;

namespace TIFF {{

{export_enum_to_cpp(TIFFType)}

template<OneOf<u32, i32> x32>
struct Rational {{
    using Type = x32;
    x32 numerator;
    x32 denominator;

    double as_double() const {{
        return static_cast<double>(numerator) / denominator;

Review comment (suggestion):
very nit: you could include that in a common variable like `LICENSE` renamed to `HEADER`.

Code after:
private:
    friend {HANDLE_TAG_SIGNATURE_TIFF_NAMESPACE};

    virtual void fill_main_tags() const override {{
        if (model().has_value())
            m_main_tags.set("Model"sv, model().value());
        if (make().has_value())
            m_main_tags.set("Manufacturer"sv, make().value());
        if (software().has_value())
            m_main_tags.set("Software"sv, software().value());
        if (date_time().has_value())
            m_main_tags.set("Creation Time"sv, date_time().value());
        if (artist().has_value())
            m_main_tags.set("Author"sv, artist().value());
    }}

    void add_entry(StringView key, Vector<TIFF::Value>&& value) {{
        m_data.set(key, move(value));
    }}

    HashMap<StringView, Vector<TIFF::Value>> m_data;
}};
"""

    return output


def generate_metadata_file(tags: List[Tag]) -> str:
    output = fR"""{HEADER}

#pragma once

#include <AK/HashMap.h>
#include <AK/Variant.h>
#include <AK/Vector.h>
#include <LibGfx/Size.h>
#include <LibGfx/ImageFormats/ImageDecoder.h>

namespace Gfx {{

class ExifMetadata;

namespace TIFF {{

{export_enum_to_cpp(TIFFType)}

template<OneOf<u32, i32> x32>
struct Rational {{
    using Type = x32;
    x32 numerator;
    x32 denominator;
Repository: SerenityOS/serenity
File: Kernel/Syscalls/posix_spawn.cpp (C++)

Code before:
        });
    });

    dbgln_if(FORK_DEBUG, "posix_spawn: child={}", child);

    // A child created via posix_spawn inherits a copy of its parent's signal mask
    child_first_thread->update_signal_mask(Thread::current()->signal_mask());

    Thread* new_main_thread = nullptr;
    InterruptsState previous_interrupts_state = InterruptsState::Enabled;
    TRY(child->exec(move(path), move(arguments), move(environment), new_main_thread, previous_interrupts_state));
    thread_finalizer_guard.disarm();

    m_scoped_process_list.with([&](auto const& list_ptr) {
        if (list_ptr) {
            child->m_scoped_process_list.with([&](auto& child_list_ptr) {
                child_list_ptr = list_ptr;
            });
            list_ptr->attach(*child);
        }
    });

    PerformanceManager::add_process_created_event(*child);

    SpinlockLocker lock(g_scheduler_lock);
    new_main_thread->set_affinity(Thread::current()->affinity());
    new_main_thread->set_state(Thread::State::Runnable);

    return child->pid().value();
}
}


Review comment (bug):
While this does make the code for creating userspace processes a lot less repetitive, the process should only be registered (and have a leaked ref on it) once we are sure that we won't return an error after this.

If you do it like this, we will leak processes when sys$posix_spawn fails, since you already leaked a ref on them. (And I also think other stuff might break, but I don't want to think of all edge cases right now.)

Registering processes also causes them to be visible in stuff like SystemMonitor, so you can see partially created processes there, which is probably not what we want.

(If we lived in a world where fork() didn't exist, we wouldn't have to worry about all of this and creating a process and exec() would be a single step.)

Code after:
        });
    });

    dbgln_if(FORK_DEBUG, "posix_spawn: child={}", child);

    // A child created via posix_spawn inherits a copy of its parent's signal mask
    child_first_thread->update_signal_mask(Thread::current()->signal_mask());

    Thread* new_main_thread = nullptr;
    InterruptsState previous_interrupts_state = InterruptsState::Enabled;
    TRY(child->exec(move(path), move(arguments), move(environment), new_main_thread, previous_interrupts_state, ProcessEventType::Create));
    thread_finalizer_guard.disarm();

    m_scoped_process_list.with([&](auto const& list_ptr) {
        if (list_ptr) {
            child->m_scoped_process_list.with([&](auto& child_list_ptr) {
                child_list_ptr = list_ptr;
            });
            list_ptr->attach(*child);
        }
    });

    commit_creation(child);

    SpinlockLocker lock(g_scheduler_lock);
    new_main_thread->set_affinity(Thread::current()->affinity());
    new_main_thread->set_state(Thread::State::Runnable);

    return child->pid().value();
}
}
Repository: SerenityOS/serenity
File: Kernel/Syscalls/fork.cpp (C++)

Code before:
                TRY(child_space->region_tree().place_specifically(*region_clone, region.range()));
                (void)region_clone.leak_ptr();
            }
            return {};
        });
    }));

    thread_finalizer_guard.disarm();

    m_scoped_process_list.with([&](auto& list_ptr) {
        if (list_ptr) {
            child->m_scoped_process_list.with([&](auto& child_list_ptr) {
                child_list_ptr = list_ptr;
            });
            list_ptr->attach(*child);
        }
    });

    SpinlockLocker lock(g_scheduler_lock);
    child_first_thread->set_affinity(Thread::current()->affinity());
    child_first_thread->set_state(Thread::State::Runnable);

    auto child_pid = child->pid().value();

    return child_pid;
}
}


Review comment (bug):
Similar reason as to the `Process::register_new()` change here. This causes us to send the process created event a lot earlier, but `fork()`ing might still fail after that, leading us to emitting an event for a process that never existed.

Code after:
                TRY(child_space->region_tree().place_specifically(*region_clone, region.range()));
                (void)region_clone.leak_ptr();
            }
            return {};
        });
    }));

    thread_finalizer_guard.disarm();

    m_scoped_process_list.with([&](auto& list_ptr) {
        if (list_ptr) {
            child->m_scoped_process_list.with([&](auto& child_list_ptr) {
                child_list_ptr = list_ptr;
            });
            list_ptr->attach(*child);
        }
    });

    commit_creation(child);

    SpinlockLocker lock(g_scheduler_lock);
    child_first_thread->set_affinity(Thread::current()->affinity());
    child_first_thread->set_state(Thread::State::Runnable);

    auto child_pid = child->pid().value();

    return child_pid;
}
}
Repository: SerenityOS/serenity
File: Tests/LibGfx/test-inputs/jbig2/json/compile.sh (Shell)

Code before:
ROOT="$DIR/../../../../.."
LAGOM_BUILD="$ROOT/Build/lagom"
JBIG2_FROM_JSON="$LAGOM_BUILD/bin/jbig2-from-json"

export DIR
export JBIG2_FROM_JSON

run_jbig2() {
    f="$1"
    filename=$(basename "${f%.json}.jbig2")
    f_jb2="$DIR/../$filename"

    if ! output=$("$JBIG2_FROM_JSON" -o "$f_jb2" "$f" 2>&1); then
        echo failed to run:
        echo "$JBIG2_FROM_JSON" -o "$f_jb2" "$f"
        echo "$output"
        return 1
    fi
}

. "$ROOT/Meta/shell_include.sh"
NPROC=$(get_number_of_processing_units)

export -f run_jbig2
find "$DIR" -maxdepth 1 -name "*.json" -print0 | \
    xargs -0 -P "$NPROC" -I {} bash -c 'run_jbig2 "$@"' _ {}


Review comment (suggestion):
So this may change with the other comment,
But `-I` implies `-n1` so the `-print0`/`-0` are not necessary

Code after:
ROOT="$DIR/../../../../.."
LAGOM_BUILD="$ROOT/Build/lagom"
JBIG2_FROM_JSON=${1:-"$LAGOM_BUILD/bin/jbig2-from-json"}

export DIR
export JBIG2_FROM_JSON

run_jbig2() {
    f="$1"
    filename=$(basename "${f%.json}.jbig2")
    f_jb2="$DIR/../$filename"

    if ! output=$("$JBIG2_FROM_JSON" -o "$f_jb2" "$f" 2>&1); then
        echo failed to run:
        echo "$JBIG2_FROM_JSON" -o "$f_jb2" "$f"
        echo "$output"
        return 1
    fi
}

. "$ROOT/Meta/shell_include.sh"
NPROC=$(get_number_of_processing_units)

export -f run_jbig2
find "$DIR" -maxdepth 1 -name "*.json" -print0 | \
    xargs -0 -P "$NPROC" -I {} bash -c 'run_jbig2 "$@"' _ {}
Repository: SerenityOS/serenity
File: Kernel/Arch/aarch64/Processor.cpp (C++)

Code before:
    auto in_critical = to_thread->saved_critical();
    VERIFY(in_critical > 0);
    Processor::restore_critical(in_critical);

    Processor::load_fpu_state(to_thread->fpu_state());

    if (from_thread->process().is_traced())
        read_debug_registers_into(from_thread->debug_register_state());

    if (to_thread->process().is_traced()) {
        write_debug_registers_from(to_thread->debug_register_state());
    } else {
        clear_debug_registers();
    }
}

template<typename T>
StringView ProcessorBase<T>::platform_string()
{
    return "aarch64"sv;
}

template<typename T>
void ProcessorBase<T>::idle() const
{
    // This function is expected to be called with interrupts disabled.

    idle_begin();
    asm volatile(R"(
        // Go to sleep. Execution will continue when the processor receives an interrupt, even if interrupts are disabled.
        // The processor may also be woken up by other implementation defined wake events, in that case we return from this function without an interrupt.
        wfi

        // Shortly unmask interrupts to call the interrupt handler for the received interrupt.
        msr daifclr, #2 // PSTATE.I = 0
        msr daifset, #2 // PSTATE.I = 1
    )" :);
    idle_end();
}

template<typename T>
Processor& ProcessorBase<T>::by_id(u32 id)
{
    (void)id;
    TODO_AARCH64();
}

}

#include <Kernel/Arch/ProcessorFunctions.include>


Review comment (suggestion):
Can't we use `VERIFY_INTERRUPTS_DISABLED()`?
Or maybe we need `VERIFY(T::::are_interrupts_enabled())`
(Same for all architectures)

Code after:
StringView ProcessorBase::platform_string()
{
    return "aarch64"sv;
}

void ProcessorBase::idle() const
{
    VERIFY_INTERRUPTS_DISABLED();

    idle_begin();
    asm volatile(R"(
        // Go to sleep. Execution will continue when the processor receives an interrupt, even with interrupts disabled.
        // The processor may also be woken up by other implementation defined wake events, in that case we return from this function without an interrupt.
        wfi

        // Shortly unmask interrupts to call the interrupt handler for the received interrupt.
        msr daifclr, #2 // PSTATE.I = 0
        msr daifset, #2 // PSTATE.I = 1
    )" :);
    idle_end();
}

Processor& ProcessorBase::by_id(u32 id)
{
    (void)id;
    TODO_AARCH64();
}

void ProcessorBase::idle_begin() const
{
    // FIXME: Implement this when SMP for aarch64 is supported.
}

void ProcessorBase::idle_end() const
{
    // FIXME: Implement this when SMP for aarch64 is supported.
}

void ProcessorBase::smp_enable()
{
    // FIXME: Implement this when SMP for aarch64 is supported.
}

bool ProcessorBase::is_smp_enabled()
{
    return false;
}

}
Repository: SerenityOS/serenity
File: Kernel/Memory/MappedROM.h (C/C++)

Code before:

#include <AK/OwnPtr.h>
#include <Kernel/Memory/PhysicalAddress.h>
#include <Kernel/Memory/Region.h>

namespace Kernel::Memory {

class MappedROM {
public:
    u8 const* base() const { return region->vaddr().offset(offset).as_ptr(); }
    u8 const* end() const { return base() + size; }
    OwnPtr<Region> region;
    size_t size { 0 };
    size_t offset { 0 };
    PhysicalAddress paddr;

    u8 const* pointer_to_chunk_starting_with(StringView prefix, size_t chunk_size) const
    {
        auto prefix_length = prefix.length();
        if (size < prefix_length)
            return {};
        for (auto* candidate = base(); candidate <= end() - prefix_length; candidate += chunk_size) {
            if (!__builtin_memcmp(prefix.characters_without_null_termination(), candidate, prefix.length()))
                return candidate;
        }
        return nullptr;
    }

    Optional<PhysicalAddress> find_chunk_starting_with(StringView prefix, size_t chunk_size) const
    {
        auto result = pointer_to_chunk_starting_with(prefix, chunk_size);
        if (!result)
            return {};
        return paddr_of(result);
    }

    PhysicalAddress paddr_of(u8 const* ptr) const { return paddr.offset(ptr - this->base()); }
};

}


Review comment (bug):
I think we'd usually use a `Optional` here instead of returning a `nullptr`.

Code after:

#include <AK/OwnPtr.h>
#include <Kernel/Memory/PhysicalAddress.h>
#include <Kernel/Memory/Region.h>

namespace Kernel::Memory {

class MappedROM {
public:
    u8 const* base() const { return region->vaddr().offset(offset).as_ptr(); }
    u8 const* end() const { return base() + size; }
    OwnPtr<Region> region;
    size_t size { 0 };
    size_t offset { 0 };
    PhysicalAddress paddr;

    u8 const* pointer_to_chunk_starting_with(StringView prefix, size_t chunk_size) const
    {
        auto prefix_length = prefix.length();
        if (size < prefix_length)
            return nullptr;
        for (auto* candidate = base(); candidate <= end() - prefix_length; candidate += chunk_size) {
            if (!__builtin_memcmp(prefix.characters_without_null_termination(), candidate, prefix.length()))
                return candidate;
        }
        return nullptr;
    }

    Optional<PhysicalAddress> find_chunk_starting_with(StringView prefix, size_t chunk_size) const
    {
        auto result = pointer_to_chunk_starting_with(prefix, chunk_size);
        if (!result)
            return {};
        return paddr_of(result);
    }

    PhysicalAddress paddr_of(u8 const* ptr) const { return paddr.offset(ptr - this->base()); }
};

}